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
| ||
|
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
Powered by blists - more mailing lists