linux-kernel - Re: [alsa-devel] [PATCH v4 06/15] soundwire: Add IO transfer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8c38baee-45f8-682e-f781-c35e26c26d9f@linux.intel.com>
Date:   Wed, 6 Dec 2017 07:32:43 -0600
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Vinod Koul <vinod.koul@...el.com>
Cc:     ALSA <alsa-devel@...a-project.org>,
        Charles Keepax <ckeepax@...nsource.cirrus.com>,
        Takashi <tiwai@...e.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        plai@...eaurora.org, LKML <linux-kernel@...r.kernel.org>,
        Sagar Dharia <sdharia@...eaurora.org>, patches.audio@...el.com,
        Mark <broonie@...nel.org>, srinivas.kandagatla@...aro.org,
        Sudheer Papothi <spapothi@...eaurora.org>, alan@...ux.intel.com
Subject: Re: [alsa-devel] [PATCH v4 06/15] soundwire: Add IO transfer

On 12/5/17 11:58 PM, Vinod Koul wrote:
> On Tue, Dec 05, 2017 at 08:48:03AM -0600, Pierre-Louis Bossart wrote:
>> On 12/5/17 7:43 AM, Pierre-Louis Bossart wrote:
>>> On 12/5/17 12:31 AM, Vinod Koul wrote:
>>>> On Sun, Dec 03, 2017 at 09:01:41PM -0600, Pierre-Louis Bossart wrote:
> 
>>>>>>>> +static inline int do_transfer(struct sdw_bus *bus, struct
>>>>>>>> sdw_msg *msg)
>>>>>>>> +{
>>>>>>>> +    int retry = bus->prop.err_threshold;
>>>>>>>> +    enum sdw_command_response resp;
>>>>>>>> +    int ret = 0, i;
>>>>>>>> +
>>>>>>>> +    for (i = 0; i <= retry; i++) {
>>>>>>>> +        resp = bus->ops->xfer_msg(bus, msg);
>>>>>>>> +        ret = find_response_code(resp);
>>>>>>>> +
>>>>>>>> +        /* if cmd is ok or ignored return */
>>>>>>>> +        if (ret == 0 || ret == -ENODATA)
>>>>>>>
>>>>>>> Can you document why you don't retry on a CMD_IGNORED? I know
>>>>>>> there was a
>>>>>>> reason, I just can't remember it.
>>>>>>
>>>>>> CMD_IGNORED can be okay on broadcast. User of this API can retry all
>>>>>> they
>>>>>> want!
>>>>>
>>>>> So you retry if this is a CMD_FAILED but let higher levels retry for
>>>>> CMD_IGNORED, sorry I don't see the logic.
>>>>
>>>> Yes that is right.
>>>>
>>>> If I am doing a broadcast read, lets say for Device Id registers, why in
>>>> the
>>>> world would I want to retry? CMD_IGNORED is a valid response and
>>>> required to
>>>> stop enumeration cycle in that case.
> 
> Above is the clarfication
> 
>>>> But if I am not expecting a CMD_IGNORED response, I can very well go
>>>> ahead
>>>> and retry from caller. The context is with caller and they can choose to
>>>> do
>>>> appropriate handling.
>>>>
>>>> And I have clarified this couple of times to you already, not sure how
>>>> many
>>>> more times I would have to do that.
>>>
>>> Until you clarify what you are doing.
> 
> Let me try again, I think u missed that part of my reply above
> 
> If I am doing a broadcast read, lets say for Device Id registers,
> why in the world would I want to retry? CMD_IGNORED is a valid response
> and required to stop enumeration cycle in that case.
> 
>>> There is ONE case where IGNORED is a valid answer (reading the Prepare not
>>> finished bits), and it should not only be documented but analyzed in more
>>> details.
>> I meant Read SCP_DevID registers from Device0... prepare bits should never
>> return a CMD_IGNORED
> 
> Precisely as I pointed out above.
> 
>>> For a write an IGNORED is never OK.
> 
> Agreed, but then transfer does both read and write. Write call can treat it
> as error and read call leaves it upto caller.
> 
> Does that sound okay for you?

Not really.
You have one case where it's ok and all others are not ok, to me this 
sounds like you should avoid the retry at the lowest level rather than 
pushing this to the caller
And now that I think of it, the definitions for the DisCo spec were 
really meant for hardware-based retries available in some master IPs. If 
this is enabled, then the loops in software are not really needed at 
all. Can you please check this point?