Re: [PATCH 2/2] enhance sysfs rfkill interface

[Dmitry Torokhov]

On Sat, Mar 13, 2010 at 10:41:28AM +0100, Florian Mickler wrote:
> On Fri, 12 Mar 2010 14:48:28 -0800
> Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote:
> 
> > On Fri, Mar 12, 2010 at 11:39:25PM +0100, Florian Mickler wrote:
> > > On Fri, 12 Mar 2010 13:20:26 -0800
> > > Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote:
> > > 
> > > > On Fri, Mar 12, 2010 at 09:57:43PM +0100, Florian Mickler wrote:
> > > > > On Fri, 12 Mar 2010 10:22:09 -0800
> > > > > Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote:
> > > > > 
> > > > > > On Fri, Mar 12, 2010 at 07:03:08PM +0100, florian@xxxxxxxxxxx wrote:
> > > > > > >  
> > > > > > > +static ssize_t rfkill_hard_show(struct device *dev,
> > > > > > > +				 struct device_attribute *attr,
> > > > > > > +				 char *buf)
> > > > > > > +{
> > > > > > > +	struct rfkill *rfkill = to_rfkill(dev);
> > > > > > > +	unsigned long flags;
> > > > > > > +	u32 state;
> > > > > > > +
> > > > > > > +	spin_lock_irqsave(&rfkill->lock, flags);
> > > > > > > +	state = rfkill->state;
> > > > > > > +	spin_unlock_irqrestore(&rfkill->lock, flags);
> > > > > > 
> > > > > > Why exactly is this lock needed?
> > > > > 
> > > > > The rfkill state is updated from multiple contexts... Am I overlooking
> > > > > smth obvious here?
> > > > > 
> > > > 
> > > > You are not updating but reading... Are you concerned about seeing
> > > > a partial write to u32? It does not happen.
> > > > 
> > > Hm.. You shure? On every arch that supports wireless drivers? 
> > > 
> > > I've just copied that code from the old sysfs state-file handler.
> > > So I assumed that reading partial updated state can happen... Also I
> > > just searched a little but did not find anything, cause i didn't know
> > > where to look. Who garantees this? Is it a gcc thing? 
> > > 
> > 
> > None of the arches would do byte-by-byte writes to a u32, they'd write
> > dword at once. Also, even if they could, you are interested in a single
> > flag (bit). You do realize that once you leave spinlock whatever you
> > fetched is stale data and may not be trusted?
> 
> On Fri, 12 Mar 2010 18:48:19 -0500
> Valdis.Kletnieks@xxxxxx wrote:
> > If a u32 load or store from memory isn't atomic, the Linux kernel is screwed
> > anyhow.  Hint - imagine if every 32-bit reference had to be treated the way
> > we currently treat 64-bit references on a 32-bit system.
> 
> 
> i presume, there is no way any digital device could write _one bit_
> partial :) 
> so this _may_ actually be safe *g* 
>  
> how about the write in the _store() function? there we
> read,update and write back the whole 32 bit which then potentially
> overwrites some other flag concurrently set by an driver interrupt on
> another cpu? i think the lock there is needed.
>

Well, right now it is mutex so it will not protect if something happens
in interrupt context. Takeing the global rfkill mutex seems pretty heavy
but there does not seem to be a per-device mutex. There also some
muching with spinlock inside rfkill_set_state but it is dropped when we
actually carry out the operation. I am afraid the locking in rfkill
needs some reviewing...

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/