Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1501759 > unrolled thread

Re: [PATCH v4 2/8] scpi: Add alternative legacy structures, functions and macros

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 2/8] scpi: Add alternative legacy structures, functions  and macros Neil Armstrong <narmstrong@baylibre.com> - 2016-10-17 10:30 +0200
    Re: [PATCH v4 2/8] scpi: Add alternative legacy structures, functions  and macros Sudeep Holla <sudeep.holla@arm.com> - 2016-10-17 13:20 +0200
      Re: [PATCH v4 2/8] scpi: Add alternative legacy structures, functions  and macros Neil Armstrong <narmstrong@baylibre.com> - 2016-10-19 16:40 +0200
        Re: [PATCH v4 2/8] scpi: Add alternative legacy structures, functions  and macros Sudeep Holla <sudeep.holla@arm.com> - 2016-10-19 18:20 +0200
        Re: [PATCH v4 2/8] scpi: Add alternative legacy structures, functions  and macros Sudeep Holla <sudeep.holla@arm.com> - 2016-10-19 18:30 +0200

#1501759 — Re: [PATCH v4 2/8] scpi: Add alternative legacy structures, functions and macros

On 10/10/2016 04:36 PM, Sudeep Holla wrote:
> Hi Neil,
> 
> Sorry, I could not reply to your response on v3. Anyways I will review v4.
> 
> On 05/10/16 08:33, Neil Armstrong wrote:
>> This patch adds support for the Legacy SCPI protocol in early JUNO versions and
>> shipped Amlogic ARMv8 based SoCs. Some Rockchip SoC are also known to use this
>> version of protocol with extended vendor commands
>> .
>> In order to support the legacy SCPI protocol variant, add back the structures
>> and macros that varies against the final specification.
>> Then add indirection table for legacy commands.
>> Finally Add bitmap field for channel selection since the Legacy protocol mandates to
>> send a selected subset of the commands on the high priority channel instead of the
>> low priority channel.
>>
>> The message sending path differs from the final SCPI procotocol because the
>> Amlogic SCP firmware always reply 1 instead of a special value containing the command
>> byte and replied rx data length.
>> For this reason commands queuing cannot be used and we assume the reply command is
>> the head of the rx_pending list since we ensure sequential command sending with a
>> separate dedicated mutex.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/firmware/arm_scpi.c | 221 +++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 199 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>> index 498afa0..6244eb1 100644
>> --- a/drivers/firmware/arm_scpi.c
>> +++ b/drivers/firmware/arm_scpi.c
> 
> [...]
> 
>> @@ -307,21 +398,46 @@ static void scpi_process_cmd(struct scpi_chan *ch, u32 cmd)
>>          return;
>>      }
>>
>> -    list_for_each_entry(t, &ch->rx_pending, node)
>> -        if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {
>> -            list_del(&t->node);
>> -            match = t;
>> -            break;
>> -        }
>> +    /* Command type is not replied by the SCP Firmware in legacy Mode
>> +     * We should consider that command is the head of pending RX commands
>> +     * if the list is not empty. In TX only mode, the list would be empty.
>> +     */
>> +    if (scpi_info->is_legacy) {
>> +        match = list_first_entry(&ch->rx_pending, struct scpi_xfer,
>> +                     node);
>> +        list_del(&match->node);
>> +    } else {
>> +        list_for_each_entry(t, &ch->rx_pending, node)
>> +            if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {
>> +                list_del(&t->node);
>> +                match = t;
>> +                break;
>> +            }
>> +    }
>>      /* check if wait_for_completion is in progress or timed-out */
>>      if (match && !completion_done(&match->done)) {
>> -        struct scpi_shared_mem *mem = ch->rx_payload;
>> -        unsigned int len = min(match->rx_len, CMD_SIZE(cmd));
>> +        unsigned int len;
>> +
>> +        if (scpi_info->is_legacy) {
>> +            struct legacy_scpi_shared_mem *mem = ch->rx_payload;
>> +
>> +            /* RX Length is not replied by the lagcy Firmware */
>> +            len = match->rx_len;
>> +
>> +            match->status = le32_to_cpu(mem->status);
>> +            memcpy_fromio(match->rx_buf, mem->payload, len);
> 
> The above 2 seems common to both, no ?

No, the shared_mem structure differs.

> 
>> +        } else {
>> +            struct scpi_shared_mem *mem = ch->rx_payload;
>> +
>> +            len = min(match->rx_len, CMD_SIZE(cmd));
>> +
>> +            match->status = le32_to_cpu(mem->status);
>> +            memcpy_fromio(match->rx_buf, mem->payload, len);
>> +        }
>>
>> -        match->status = le32_to_cpu(mem->status);
>> -        memcpy_fromio(match->rx_buf, mem->payload, len);
>>          if (match->rx_len > len)
>>              memset(match->rx_buf + len, 0, match->rx_len - len);
>> +
> 
> Spurious ?

Yep

> 
>>          complete(&match->done);
>>      }
>>      spin_unlock_irqrestore(&ch->rx_lock, flags);
>> @@ -331,7 +447,12 @@ static void scpi_handle_remote_msg(struct mbox_client *c, void *msg)
>>  {
>>      struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
>>      struct scpi_shared_mem *mem = ch->rx_payload;
>> -    u32 cmd = le32_to_cpu(mem->command);
>> +    u32 cmd;
>> +
>> +    if (scpi_info->is_legacy)
>> +        cmd = *(u32 *)msg;
> 
> Do we need do this if it doesn't contain command ?

No, will remove.

> 
>> +    else
>> +        cmd = le32_to_cpu(mem->command);
>>
>>      scpi_process_cmd(ch, cmd);
>>  }
>> @@ -343,17 +464,26 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
>>      struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
>>      struct scpi_shared_mem *mem = (struct scpi_shared_mem *)ch->tx_payload;
>>
>> -    if (t->tx_buf)
>> -        memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
>> +    if (t->tx_buf) {
>> +        if (scpi_info->is_legacy)
>> +            memcpy_toio(ch->tx_payload, t->tx_buf, t->tx_len);
>> +        else
>> +            memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
>> +    }
>> +
>>      if (t->rx_buf) {
>>          if (!(++ch->token))
>>              ++ch->token;
>>          ADD_SCPI_TOKEN(t->cmd, ch->token);
>> +        if (scpi_info->is_legacy)
>> +            t->slot = t->cmd;
> 
> I thought passing token was not an issue from your previous response,
> but you are overriding it here, why ?

Indeed, I can leave it, but it's useless since it won't serve to distinguish multiple similar commands.

> 
>>          spin_lock_irqsave(&ch->rx_lock, flags);
>>          list_add_tail(&t->node, &ch->rx_pending);
>>          spin_unlock_irqrestore(&ch->rx_lock, flags);
>>      }
>> -    mem->command = cpu_to_le32(t->cmd);
>> +
>> +    if (!scpi_info->is_legacy)
>> +        mem->command = cpu_to_le32(t->cmd);
>>  }
>>
>>  static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)
>> @@ -396,21 +526,37 @@ static int scpi_send_message(unsigned int offset, void *tx_buf,
>>
>>      cmd = scpi_info->scpi_cmds[offset];
>>
>> -    chan = atomic_inc_return(&scpi_info->next_chan) % scpi_info->num_chans;
>> +    if (scpi_info->is_legacy)
>> +        chan = test_bit(cmd, scpi_info->cmd_priority) ? 1 : 0;
>> +    else
>> +        chan = atomic_inc_return(&scpi_info->next_chan) %
>> +            scpi_info->num_chans;
>>      scpi_chan = scpi_info->channels + chan;
>>
>>      msg = get_scpi_xfer(scpi_chan);
>>      if (!msg)
>>          return -ENOMEM;
>>
>> -    msg->slot = BIT(SCPI_SLOT);
>> -    msg->cmd = PACK_SCPI_CMD(cmd, tx_len);
>> +    if (scpi_info->is_legacy) {
>> +        msg->cmd = PACK_LEGACY_SCPI_CMD(cmd, tx_len);
>> +        msg->slot = msg->cmd;
>> +    } else {
>> +        msg->slot = BIT(SCPI_SLOT);
>> +        msg->cmd = PACK_SCPI_CMD(cmd, tx_len);
>> +    }
>>      msg->tx_buf = tx_buf;
>>      msg->tx_len = tx_len;
>>      msg->rx_buf = rx_buf;
>>      msg->rx_len = rx_len;
>>      init_completion(&msg->done);
>>
>> +    /* Since we cannot distinguish the original command in the
>> +     * MHU reply stat value from a Legacy SCP firmware, ensure
>> +     * sequential command sending to the firmware.
>> +     */
> 
> OK this comment now questions the existence of this extra lock.
> The mailbox will always send the commands in the sequential order.
> It's only firmware that can re-order the response. Since that can't
> happen in you case, I really don't see the need for this.
> 
> Please explain the race you would see without this locking. Yes I
> understand that only one command is supposed to be sent to firmware at a
> time. Suppose you allow more callers here, all will wait on the
> completion flags and the first in the list gets unblocked right ?
> I am just trying to understand if there's real need for this extra
> lock when we already have that from the list.

In my current tests I have huge kernel hang when having multiple callers,
I must find out where this issue comes from...
In any case, we have an issue about the command sequencing.
If we push a tx-only command and then right after a tx-rx command, the
mailbox callback from the first command won't be able to distinguish which
command is handled !
In this case, the rx_pending list will not be empty, some garbage will be returned
to the second command handler and the real data from the second command handling
will be lost thinking it's a tx-only command.


We have two choices here :
 - Also push the tx-only commands to the rx_pending list, and also wait for their completion
 - Add an extra lock

What is your preferred scheme ?

>> +    if (scpi_info->is_legacy)
>> +        mutex_lock(&scpi_chan->legacy_lock);
>> +
>>      ret = mbox_send_message(scpi_chan->chan, msg);
>>      if (ret < 0 || !rx_buf)
>>          goto out;
>> @@ -421,9 +567,13 @@ static int scpi_send_message(unsigned int offset, void *tx_buf,
>>          /* first status word */
>>          ret = msg->status;
>>  out:
>> -    if (ret < 0 && rx_buf) /* remove entry from the list if timed-out */
>> +    if (ret < 0 && rx_buf)
>> +        /* remove entry from the list if timed-out */
>>          scpi_process_cmd(scpi_chan, msg->cmd);
>>
>> +    if (scpi_info->is_legacy)
>> +        mutex_unlock(&scpi_chan->legacy_lock);
>> +
>>      put_scpi_xfer(msg, scpi_chan);
>>      /* SCPI error codes > 0, translate them to Linux scale*/
>>      return ret > 0 ? scpi_to_linux_errno(ret) : ret;
> 
> [...]
> 
>> @@ -525,7 +687,6 @@ static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
>>
>>      info->count = DVFS_OPP_COUNT(buf.header);
>>      info->latency = DVFS_LATENCY(buf.header) * 1000; /* uS to nS */
>> -
> 
> Spurious ?

Indeed.

> 
>>      info->opps = kcalloc(info->count, sizeof(*opp), GFP_KERNEL);
>>      if (!info->opps) {
>>          kfree(info);
>> @@ -580,9 +741,13 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)
>>
>>      ret = scpi_send_message(CMD_SENSOR_VALUE, &id, sizeof(id),
>>                  &buf, sizeof(buf));
>> -    if (!ret)
>> -        *val = (u64)le32_to_cpu(buf.hi_val) << 32 |
>> -            le32_to_cpu(buf.lo_val);
>> +    if (!ret) {
>> +        if (scpi_info->is_legacy)
>> +            *val = (u64)le32_to_cpu(buf.lo_val);
>> +        else
>> +            *val = (u64)le32_to_cpu(buf.hi_val) << 32 |
>> +                le32_to_cpu(buf.lo_val);
>> +    }
> 
> Not required as I have mentioned couple of times in previous versions,
> it's zero filled by the driver.
> 

OK

I will fix the issues, but I need your advice for the locking scheme. I really want this
to be merged and be able to go forward !

Thanks,
Neil

[toc] | [next] | [standalone]

#1501884

On 17/10/16 09:25, Neil Armstrong wrote:
> On 10/10/2016 04:36 PM, Sudeep Holla wrote:
>> Hi Neil,
>>
>> Sorry, I could not reply to your response on v3. Anyways I will review v4.
>>
>> On 05/10/16 08:33, Neil Armstrong wrote:
>>> This patch adds support for the Legacy SCPI protocol in early JUNO versions and
>>> shipped Amlogic ARMv8 based SoCs. Some Rockchip SoC are also known to use this
>>> version of protocol with extended vendor commands
>>> .
>>> In order to support the legacy SCPI protocol variant, add back the structures
>>> and macros that varies against the final specification.
>>> Then add indirection table for legacy commands.
>>> Finally Add bitmap field for channel selection since the Legacy protocol mandates to
>>> send a selected subset of the commands on the high priority channel instead of the
>>> low priority channel.
>>>
>>> The message sending path differs from the final SCPI procotocol because the
>>> Amlogic SCP firmware always reply 1 instead of a special value containing the command
>>> byte and replied rx data length.
>>> For this reason commands queuing cannot be used and we assume the reply command is
>>> the head of the rx_pending list since we ensure sequential command sending with a
>>> separate dedicated mutex.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>>  drivers/firmware/arm_scpi.c | 221 +++++++++++++++++++++++++++++++++++++++-----
>>>  1 file changed, 199 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>>> index 498afa0..6244eb1 100644
>>> --- a/drivers/firmware/arm_scpi.c
>>> +++ b/drivers/firmware/arm_scpi.c
>>
>> [...]
>>
>>> @@ -307,21 +398,46 @@ static void scpi_process_cmd(struct scpi_chan *ch, u32 cmd)
>>>          return;
>>>      }
>>>
>>> -    list_for_each_entry(t, &ch->rx_pending, node)
>>> -        if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {
>>> -            list_del(&t->node);
>>> -            match = t;
>>> -            break;
>>> -        }
>>> +    /* Command type is not replied by the SCP Firmware in legacy Mode
>>> +     * We should consider that command is the head of pending RX commands
>>> +     * if the list is not empty. In TX only mode, the list would be empty.
>>> +     */
>>> +    if (scpi_info->is_legacy) {
>>> +        match = list_first_entry(&ch->rx_pending, struct scpi_xfer,
>>> +                     node);
>>> +        list_del(&match->node);
>>> +    } else {
>>> +        list_for_each_entry(t, &ch->rx_pending, node)
>>> +            if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {
>>> +                list_del(&t->node);
>>> +                match = t;
>>> +                break;
>>> +            }
>>> +    }
>>>      /* check if wait_for_completion is in progress or timed-out */
>>>      if (match && !completion_done(&match->done)) {
>>> -        struct scpi_shared_mem *mem = ch->rx_payload;
>>> -        unsigned int len = min(match->rx_len, CMD_SIZE(cmd));
>>> +        unsigned int len;
>>> +
>>> +        if (scpi_info->is_legacy) {
>>> +            struct legacy_scpi_shared_mem *mem = ch->rx_payload;
>>> +
>>> +            /* RX Length is not replied by the lagcy Firmware */

Typo above legacy

>>> +            len = match->rx_len;
>>> +
>>> +            match->status = le32_to_cpu(mem->status);
>>> +            memcpy_fromio(match->rx_buf, mem->payload, len);
>>
>> The above 2 seems common to both, no ?
>
> No, the shared_mem structure differs.
>

Yes I see that, I was just referring the last 2 statements.

>>
>>> +        } else {
>>> +            struct scpi_shared_mem *mem = ch->rx_payload;
>>> +
>>> +            len = min(match->rx_len, CMD_SIZE(cmd));
>>> +
>>> +            match->status = le32_to_cpu(mem->status);
>>> +            memcpy_fromio(match->rx_buf, mem->payload, len);

and the above 2 can be moved out of the conditions, no ?

if (scpi_info->is_legacy) {
	struct legacy_scpi_shared_mem *mem = ch->rx_payload;
	len = match->rx_len;
} else {
	struct scpi_shared_mem *mem = ch->rx_payload;
	len = min(match->rx_len, CMD_SIZE(cmd));
}
match->status = le32_to_cpu(mem->status);
memcpy_fromio(match->rx_buf, mem->payload, len);

should work.


[...]

>>
>>> +    else
>>> +        cmd = le32_to_cpu(mem->command);
>>>
>>>      scpi_process_cmd(ch, cmd);
>>>  }
>>> @@ -343,17 +464,26 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
>>>      struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
>>>      struct scpi_shared_mem *mem = (struct scpi_shared_mem *)ch->tx_payload;
>>>
>>> -    if (t->tx_buf)
>>> -        memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
>>> +    if (t->tx_buf) {
>>> +        if (scpi_info->is_legacy)
>>> +            memcpy_toio(ch->tx_payload, t->tx_buf, t->tx_len);
>>> +        else
>>> +            memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
>>> +    }
>>> +
>>>      if (t->rx_buf) {
>>>          if (!(++ch->token))
>>>              ++ch->token;
>>>          ADD_SCPI_TOKEN(t->cmd, ch->token);
>>> +        if (scpi_info->is_legacy)
>>> +            t->slot = t->cmd;
>>
>> I thought passing token was not an issue from your previous response,
>> but you are overriding it here, why ?
>
> Indeed, I can leave it, but it's useless since it won't serve to
> distinguish multiple similar commands.
>

OK, I don't see any point in such micro optimization, so please retain it.

[...]

>>> +    /* Since we cannot distinguish the original command in the
>>> +     * MHU reply stat value from a Legacy SCP firmware, ensure
>>> +     * sequential command sending to the firmware.
>>> +     */
>>
>> OK this comment now questions the existence of this extra lock.
>> The mailbox will always send the commands in the sequential order.
>> It's only firmware that can re-order the response. Since that can't
>> happen in you case, I really don't see the need for this.
>>
>> Please explain the race you would see without this locking. Yes I
>> understand that only one command is supposed to be sent to firmware at a
>> time. Suppose you allow more callers here, all will wait on the
>> completion flags and the first in the list gets unblocked right ?
>> I am just trying to understand if there's real need for this extra
>> lock when we already have that from the list.
>
> In my current tests I have huge kernel hang when having multiple callers,
> I must find out where this issue comes from...

Yes IMO, you should understand the root cause of this issue. There may
be issue with the existing driver itself. But just adding a lock just to
avoid the hang without understanding it is wrong.

> In any case, we have an issue about the command sequencing. If we
> push a tx-only command and then right after a tx-rx command, the
> mailbox callback from the first command won't be able to distinguish
> which command is handled !

Hmm, how exactly ? I won't expect scpi_handle_remote_msg to becalled in
that case.

> In this case, the rx_pending list will not be empty, some garbage
> will be returned to the second command handler and the real data from
> the second command handling will be lost thinking it's a tx-only
> command.
>

Yes as I said why is scpi_handle_remote_msg called for tx only command.
And more over we don't have any tx only command in the driver, I am
still unable to understand the issue you are facing. Are you sure you
have tx-only command in the failure/hang case ?

>
> We have two choices here : - Also push the tx-only commands to the
> rx_pending list, and also wait for their completion

See above, I need to know details on this tx-only command. In fact, they
may not be tx-only as SCP is sending some response back, may just status.

> - Add an extra lock
>

Not this for sure.

> What is your preferred scheme ?
>

Option 1 if it legitimate case. I mean we may be misunderstanding the
definition of tx-only command.


>>> +    if (scpi_info->is_legacy)
>>> +        mutex_lock(&scpi_chan->legacy_lock);
>>> +
>>>      ret = mbox_send_message(scpi_chan->chan, msg);
>>>      if (ret < 0 || !rx_buf)
>>>          goto out;
>>> @@ -421,9 +567,13 @@ static int scpi_send_message(unsigned int offset, void *tx_buf,
>>>          /* first status word */
>>>          ret = msg->status;
>>>  out:
>>> -    if (ret < 0 && rx_buf) /* remove entry from the list if timed-out */
>>> +    if (ret < 0 && rx_buf)
>>> +        /* remove entry from the list if timed-out */
>>>          scpi_process_cmd(scpi_chan, msg->cmd);
>>>
>>> +    if (scpi_info->is_legacy)
>>> +        mutex_unlock(&scpi_chan->legacy_lock);
>>> +
>>>      put_scpi_xfer(msg, scpi_chan);
>>>      /* SCPI error codes > 0, translate them to Linux scale*/
>>>      return ret > 0 ? scpi_to_linux_errno(ret) : ret;
>>

[...]

>
> I will fix the issues, but I need your advice for the locking scheme. I really want this
> to be merged and be able to go forward !
>

Yes I agree and I have no major concern with the series now except the
locking.

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1503725

On 10/17/2016 01:16 PM, Sudeep Holla wrote:
> 
> 
> On 17/10/16 09:25, Neil Armstrong wrote:
>> On 10/10/2016 04:36 PM, Sudeep Holla wrote:
>>> Hi Neil,
>>>
>>> Sorry, I could not reply to your response on v3. Anyways I will review v4.
>>>
>>> On 05/10/16 08:33, Neil Armstrong wrote:
>>>> This patch adds support for the Legacy SCPI protocol in early JUNO versions and
>>>> shipped Amlogic ARMv8 based SoCs. Some Rockchip SoC are also known to use this
>>>> version of protocol with extended vendor commands
>>>> .
>>>> In order to support the legacy SCPI protocol variant, add back the structures
>>>> and macros that varies against the final specification.
>>>> Then add indirection table for legacy commands.
>>>> Finally Add bitmap field for channel selection since the Legacy protocol mandates to
>>>> send a selected subset of the commands on the high priority channel instead of the
>>>> low priority channel.
>>>>
>>>> The message sending path differs from the final SCPI procotocol because the
>>>> Amlogic SCP firmware always reply 1 instead of a special value containing the command
>>>> byte and replied rx data length.
>>>> For this reason commands queuing cannot be used and we assume the reply command is
>>>> the head of the rx_pending list since we ensure sequential command sending with a
>>>> separate dedicated mutex.
>>>>
>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>>> ---
>>>>  drivers/firmware/arm_scpi.c | 221 +++++++++++++++++++++++++++++++++++++++-----
>>>>  1 file changed, 199 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>>>> index 498afa0..6244eb1 100644
>>>> --- a/drivers/firmware/arm_scpi.c
>>>> +++ b/drivers/firmware/arm_scpi.c
>>>
>>> [...]
>>>
>>>> @@ -307,21 +398,46 @@ static void scpi_process_cmd(struct scpi_chan *ch, u32 cmd)
>>>>          return;
>>>>      }
>>>>
>>>> -    list_for_each_entry(t, &ch->rx_pending, node)
>>>> -        if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {
>>>> -            list_del(&t->node);
>>>> -            match = t;
>>>> -            break;
>>>> -        }
>>>> +    /* Command type is not replied by the SCP Firmware in legacy Mode
>>>> +     * We should consider that command is the head of pending RX commands
>>>> +     * if the list is not empty. In TX only mode, the list would be empty.
>>>> +     */
>>>> +    if (scpi_info->is_legacy) {
>>>> +        match = list_first_entry(&ch->rx_pending, struct scpi_xfer,
>>>> +                     node);
>>>> +        list_del(&match->node);
>>>> +    } else {
>>>> +        list_for_each_entry(t, &ch->rx_pending, node)
>>>> +            if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {
>>>> +                list_del(&t->node);
>>>> +                match = t;
>>>> +                break;
>>>> +            }
>>>> +    }
>>>>      /* check if wait_for_completion is in progress or timed-out */
>>>>      if (match && !completion_done(&match->done)) {
>>>> -        struct scpi_shared_mem *mem = ch->rx_payload;
>>>> -        unsigned int len = min(match->rx_len, CMD_SIZE(cmd));
>>>> +        unsigned int len;
>>>> +
>>>> +        if (scpi_info->is_legacy) {
>>>> +            struct legacy_scpi_shared_mem *mem = ch->rx_payload;
>>>> +
>>>> +            /* RX Length is not replied by the lagcy Firmware */
> 
> Typo above legacy
> 
>>>> +            len = match->rx_len;
>>>> +
>>>> +            match->status = le32_to_cpu(mem->status);
>>>> +            memcpy_fromio(match->rx_buf, mem->payload, len);
>>>
>>> The above 2 seems common to both, no ?
>>
>> No, the shared_mem structure differs.
>>
> 
> Yes I see that, I was just referring the last 2 statements.
> 
>>>
>>>> +        } else {
>>>> +            struct scpi_shared_mem *mem = ch->rx_payload;
>>>> +
>>>> +            len = min(match->rx_len, CMD_SIZE(cmd));
>>>> +
>>>> +            match->status = le32_to_cpu(mem->status);
>>>> +            memcpy_fromio(match->rx_buf, mem->payload, len);
> 
> and the above 2 can be moved out of the conditions, no ?
> 
> if (scpi_info->is_legacy) {
>     struct legacy_scpi_shared_mem *mem = ch->rx_payload;
>     len = match->rx_len;
> } else {
>     struct scpi_shared_mem *mem = ch->rx_payload;
>     len = min(match->rx_len, CMD_SIZE(cmd));
> }
> match->status = le32_to_cpu(mem->status);
> memcpy_fromio(match->rx_buf, mem->payload, len);
> 
> should work.

Well, we will have "error: ‘mem’ undeclared (first use in this function)" since mem is not declared outside the if/else.

I don't see good solutions even with an union.

> 
> [...]
> 
>>>
>>>> +    else
>>>> +        cmd = le32_to_cpu(mem->command);
>>>>
>>>>      scpi_process_cmd(ch, cmd);
>>>>  }
>>>> @@ -343,17 +464,26 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
>>>>      struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
>>>>      struct scpi_shared_mem *mem = (struct scpi_shared_mem *)ch->tx_payload;
>>>>
>>>> -    if (t->tx_buf)
>>>> -        memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
>>>> +    if (t->tx_buf) {
>>>> +        if (scpi_info->is_legacy)
>>>> +            memcpy_toio(ch->tx_payload, t->tx_buf, t->tx_len);
>>>> +        else
>>>> +            memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
>>>> +    }
>>>> +
>>>>      if (t->rx_buf) {
>>>>          if (!(++ch->token))
>>>>              ++ch->token;
>>>>          ADD_SCPI_TOKEN(t->cmd, ch->token);
>>>> +        if (scpi_info->is_legacy)
>>>> +            t->slot = t->cmd;
>>>
>>> I thought passing token was not an issue from your previous response,
>>> but you are overriding it here, why ?
>>
>> Indeed, I can leave it, but it's useless since it won't serve to
>> distinguish multiple similar commands.
>>
> 
> OK, I don't see any point in such micro optimization, so please retain it.
> 

I misread my code, I leaved the token passing, but I copy back the cmd to the slot which is used by the MHU.
If I remove the "t->slot = t->cmd;", the token won't be passed to the FW.

> [...]
> 
>>>> +    /* Since we cannot distinguish the original command in the
>>>> +     * MHU reply stat value from a Legacy SCP firmware, ensure
>>>> +     * sequential command sending to the firmware.
>>>> +     */
>>>
>>> OK this comment now questions the existence of this extra lock.
>>> The mailbox will always send the commands in the sequential order.
>>> It's only firmware that can re-order the response. Since that can't
>>> happen in you case, I really don't see the need for this.
>>>
>>> Please explain the race you would see without this locking. Yes I
>>> understand that only one command is supposed to be sent to firmware at a
>>> time. Suppose you allow more callers here, all will wait on the
>>> completion flags and the first in the list gets unblocked right ?
>>> I am just trying to understand if there's real need for this extra
>>> lock when we already have that from the list.
>>
>> In my current tests I have huge kernel hang when having multiple callers,
>> I must find out where this issue comes from...
> 
> Yes IMO, you should understand the root cause of this issue. There may
> be issue with the existing driver itself. But just adding a lock just to
> avoid the hang without understanding it is wrong.
> 
>> In any case, we have an issue about the command sequencing. If we
>> push a tx-only command and then right after a tx-rx command, the
>> mailbox callback from the first command won't be able to distinguish
>> which command is handled !
> 
> Hmm, how exactly ? I won't expect scpi_handle_remote_msg to becalled in
> that case.
> 
>> In this case, the rx_pending list will not be empty, some garbage
>> will be returned to the second command handler and the real data from
>> the second command handling will be lost thinking it's a tx-only
>> command.
>>
> 
> Yes as I said why is scpi_handle_remote_msg called for tx only command.
> And more over we don't have any tx only command in the driver, I am
> still unable to understand the issue you are facing. Are you sure you
> have tx-only command in the failure/hang case ?
> 
>>
>> We have two choices here : - Also push the tx-only commands to the
>> rx_pending list, and also wait for their completion
> 
> See above, I need to know details on this tx-only command. In fact, they
> may not be tx-only as SCP is sending some response back, may just status.
> 
>> - Add an extra lock
>>
> 
> Not this for sure.
> 
>> What is your preferred scheme ?
>>
> 
> Option 1 if it legitimate case. I mean we may be misunderstanding the
> definition of tx-only command.
> 
> 
>>>> +    if (scpi_info->is_legacy)
>>>> +        mutex_lock(&scpi_chan->legacy_lock);
>>>> +
>>>>      ret = mbox_send_message(scpi_chan->chan, msg);
>>>>      if (ret < 0 || !rx_buf)
>>>>          goto out;
>>>> @@ -421,9 +567,13 @@ static int scpi_send_message(unsigned int offset, void *tx_buf,
>>>>          /* first status word */
>>>>          ret = msg->status;
>>>>  out:
>>>> -    if (ret < 0 && rx_buf) /* remove entry from the list if timed-out */
>>>> +    if (ret < 0 && rx_buf)
>>>> +        /* remove entry from the list if timed-out */
>>>>          scpi_process_cmd(scpi_chan, msg->cmd);
>>>>
>>>> +    if (scpi_info->is_legacy)
>>>> +        mutex_unlock(&scpi_chan->legacy_lock);
>>>> +
>>>>      put_scpi_xfer(msg, scpi_chan);
>>>>      /* SCPI error codes > 0, translate them to Linux scale*/
>>>>      return ret > 0 ? scpi_to_linux_errno(ret) : ret;
>>>
> 
> [...]
> 
>>
>> I will fix the issues, but I need your advice for the locking scheme. I really want this
>> to be merged and be able to go forward !
>>
> 
> Yes I agree and I have no major concern with the series now except the
> locking.
> 

[toc] | [prev] | [next] | [standalone]

#1504074

On 19/10/16 11:37, Sudeep Holla wrote:
>
>
> On 19/10/16 11:28, Neil Armstrong wrote:
>> On 10/17/2016 01:16 PM, Sudeep Holla wrote:
>
> [...]
>
>>>
>>> and the above 2 can be moved out of the conditions, no ?
>>>
>>> if (scpi_info->is_legacy) {
>>>     struct legacy_scpi_shared_mem *mem = ch->rx_payload;
>>>     len = match->rx_len;
>>> } else {
>>>     struct scpi_shared_mem *mem = ch->rx_payload;
>>>     len = min(match->rx_len, CMD_SIZE(cmd));
>>> }
>>> match->status = le32_to_cpu(mem->status);
>>> memcpy_fromio(match->rx_buf, mem->payload, len);
>>>
>>> should work.
>>
>> Well, we will have "error: ‘mem’ undeclared (first use in this
>> function)" since mem is not declared outside the if/else.
>>
>> I don't see good solutions even with an union.
>
> Right, I missed to see that. You can leave it as you had before then.
>
>
> [...]
>>>>>>      if (t->rx_buf) {
>>>>>>          if (!(++ch->token))
>>>>>>              ++ch->token;
>>>>>>          ADD_SCPI_TOKEN(t->cmd, ch->token);
>>>>>> +        if (scpi_info->is_legacy)
>>>>>> +            t->slot = t->cmd;
>>>>>
>>>>> I thought passing token was not an issue from your previous response,
>>>>> but you are overriding it here, why ?
>>>>
>>>> Indeed, I can leave it, but it's useless since it won't serve to
>>>> distinguish multiple similar commands.
>>>>
>>>
>>> OK, I don't see any point in such micro optimization, so please
>>> retain it.
>>>
>>
>> I misread my code, I leaved the token passing, but I copy back the
>> cmd  to the slot which is used by the MHU.
>> If I remove the "t->slot = t->cmd;", the token won't be passed to the
>> FW.
>>
>
> Right, sorry I think I misled you :)
>

On the other hand, if we are unable to use that in the driver as you
just have FIFO in the list for this legacy mode, I think we can drop it
completely. I know I had argued otherwise before, but that was before I
realized that we need to keep FIFO in the list ;). Let me know if you
are OK to drop it.

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1504082

On 19/10/16 11:28, Neil Armstrong wrote:
> On 10/17/2016 01:16 PM, Sudeep Holla wrote:

[...]

>>
>> and the above 2 can be moved out of the conditions, no ?
>>
>> if (scpi_info->is_legacy) {
>>     struct legacy_scpi_shared_mem *mem = ch->rx_payload;
>>     len = match->rx_len;
>> } else {
>>     struct scpi_shared_mem *mem = ch->rx_payload;
>>     len = min(match->rx_len, CMD_SIZE(cmd));
>> }
>> match->status = le32_to_cpu(mem->status);
>> memcpy_fromio(match->rx_buf, mem->payload, len);
>>
>> should work.
>
> Well, we will have "error: ‘mem’ undeclared (first use in this
> function)" since mem is not declared outside the if/else.
>
> I don't see good solutions even with an union.

Right, I missed to see that. You can leave it as you had before then.


[...]
>>>>>      if (t->rx_buf) {
>>>>>          if (!(++ch->token))
>>>>>              ++ch->token;
>>>>>          ADD_SCPI_TOKEN(t->cmd, ch->token);
>>>>> +        if (scpi_info->is_legacy)
>>>>> +            t->slot = t->cmd;
>>>>
>>>> I thought passing token was not an issue from your previous response,
>>>> but you are overriding it here, why ?
>>>
>>> Indeed, I can leave it, but it's useless since it won't serve to
>>> distinguish multiple similar commands.
>>>
>>
>> OK, I don't see any point in such micro optimization, so please retain it.
>>
>
> I misread my code, I leaved the token passing, but I copy back the
> cmd  to the slot which is used by the MHU.
> If I remove the "t->slot = t->cmd;", the token won't be passed to the
> FW.
>

Right, sorry I think I misled you :)

-- 
Regards,
Sudeep

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web