linux-kernel - Re: [alsa-devel] [PATCH v4 09/15] soundwire: Add slave status handling

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: <20171206094448.GP32417@localhost>
Date:   Wed, 6 Dec 2017 15:14:48 +0530
From:   Vinod Koul <vinod.koul@...el.com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        ALSA <alsa-devel@...a-project.org>, Mark <broonie@...nel.org>,
        Takashi <tiwai@...e.de>, patches.audio@...el.com,
        alan@...ux.intel.com,
        Charles Keepax <ckeepax@...nsource.cirrus.com>,
        Sagar Dharia <sdharia@...eaurora.org>,
        srinivas.kandagatla@...aro.org, plai@...eaurora.org,
        Sudheer Papothi <spapothi@...eaurora.org>
Subject: Re: [alsa-devel] [PATCH v4 09/15] soundwire: Add slave status
 handling

On Sun, Dec 03, 2017 at 09:52:48PM -0600, Pierre-Louis Bossart wrote:
> On 12/3/17 9:21 PM, Vinod Koul wrote:
> >On Sun, Dec 03, 2017 at 09:11:39PM -0600, Pierre-Louis Bossart wrote:
> >>On 12/3/17 11:11 AM, Vinod Koul wrote:
> >>>On Fri, Dec 01, 2017 at 05:52:03PM -0600, Pierre-Louis Bossart wrote:
> >>>
> >>>>>+		status = sdw_read(slave, SDW_DP0_INT);
> >>>>>+		if (status < 0) {
> >>>>>+			dev_err(slave->bus->dev,
> >>>>>+				"SDW_DP0_INT read failed:%d", status);
> >>>>>+			return status;
> >>>>>+		}
> >>>>>+
> >>>>>+		count++;
> >>>>>+
> >>>>>+		/* we can get alerts while processing so keep retrying */
> >>>>
> >>>>This is not incorrect, but this goes beyond what the spec requires.
> >>>>
> >>>>The additional read is to make sure some interrupts are not lost due to a
> >>>>known race condition. It would be enough to mask the status read the second
> >>>>time to only check if the interrupts sources which were cleared are still
> >>>>signaling something.
> >>>>
> >>>>With the code as it is, you may catch *new* interrupt sources, which could
> >>>>impact the arbitration/priority/policy in handling interrupts. It's not
> >>>>necessarily bad, but you'd need to document whether you want to deal with
> >>>>the race condition described in the MIPI spec or try to be smarter.
> >>>
> >>>This was based on your last comment, lets discuss more offline on this to
> >>>see what else is required here.
> >>
> >>I am fine if you leave the code as is for now, it's not bad but can be
> >>optimized.
> >
> >Not bad is not good here :)
> >
> >Okay I still havent grabbed my coffee, so help me out here. I am not sure I
> >understand here, can you point me to the part of spec handling you were
> >referring and what should be *ideally* done
> 
> You first read the status, then clear the interrupts then re-read the
> status. I'd be good enough in the second read to mask with the settings of
> the first read. This is intended to detect alert sources that fired between
> the last successful read and the write to clear interrupts (see Figure 92 in
> the 1.1 spec)
> 
> e.g.
> 
> do {
> status1= sdw_read()
> deal with interrupts
> status2 = sdw_read()
> status2 &= status1; /* filter initial sources */

Sounds better, updated now, thanks

-- 
~Vinod