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] [day] [month] [year] [list]

Message-ID: <20171206144402.GT32417@localhost>
Date:   Wed, 6 Dec 2017 20:14:02 +0530
From:   Vinod Koul <vinod.koul@...el.com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.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 Wed, Dec 06, 2017 at 07:32:43AM -0600, Pierre-Louis Bossart wrote:
> 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?

Sure I will check that

-- 
~Vinod