Re: [PATCH] leds: Add user LED driver for NIC78bx device

[Hui Chun Ong <hui.chun.ong@ni.com>]

Hi Jacek,

On Sun, 2016-10-23 at 16:12 +0200, Jacek Anaszewski wrote:
> Hi Julia,
> 
> On 10/21/2016 11:33 PM, Julia Cartwright wrote:
> > 
> > On Fri, Oct 21, 2016 at 02:56:15PM +0200, Jacek Anaszewski wrote:
> > > 
> > > Hi Hui,
> > > 
> > > Thanks for the patch. I have few comments in the code below.
> > > 
> > > On 10/21/2016 08:33 AM, Hui Chun Ong wrote:
> > > > 
> > > > Add the driver to support User LEDs on PXI Embedded Controller.
> > > > 
> > > > Signed-off-by: Hui Chun Ong <hui.chun.ong@ni.com>
> > > > Signed-off-by: Brad Mouring <brad.mouring@ni.com>
> > [..]
> > > 
> > > 
> > > > 
> > > > +
> > > > +struct ni78bx_led {
> > > > +	u8 bit;
> > > > +	u8 mask;
> > > > +	struct led_classdev cdev;
> > > > +};
> > > > +
> > > > +static inline struct ni78bx_led *to_ni78bx_led(struct
> > > > led_classdev *cdev)
> > > > +{
> > > > +	return container_of(cdev, struct ni78bx_led, cdev);
> > > > +}
> > > > +
> > > > +static void ni78bx_brightness_set(struct led_classdev *cdev,
> > > > +				  enum led_brightness
> > > > brightness)
> > > > +{
> > > > +	struct ni78bx_led *nled = to_ni78bx_led(cdev);
> > > > +	u8 value;
> > > > +
> > > > +	mutex_lock(&led_lock);
> > > You can use spin_lock_irqsave() instead, since we are not going
> > > to sleep
> > > while accessing memory mapped registers.
> > Could you explain why spin_lock_irqsave() is appropriate
> > here?  This
> > device doesn't have an interrupt to be synchronized with.  I could
> > understand spin_lock()/spin_unlock(), maybe, but why disable
> > interrupts?
> brightness_set op can be called from atomic context e.g. in case of
> timer trigger. The code that runs in the interrupt context cannot
> sleep
> due to reasons explained in [0], whereas mutex can sleep on
> contention.
> 
> spin_lock() is appropriate only in a hard irq handler.
> 
> > 
> > Is what you're saying that the set/get brightness calls can be re-
> > entered
> > by LED consumers in hardirq context?
> > 
> > The systems this driver is deployed to is much more sensitive to RT
> > execution latency than anything else, which is why I take pause to
> > unnecessary interrupt mask twiddling/preemption disabling :).
> [0] http://www.makelinux.net/ldd3/chp-5-sect-5
> 
I'm update the driver to use spin_lock_irqsave. Thanks for the
feedback.