RE: [PATCH v1 2/2] usb:cdns3 Add Cadence USB3 DRD Driver

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1151 bytes --]

Peter Chen <peter.chen@nxp.com> writes:
>> >> >> +    irqreturn_t ret = IRQ_NONE;
>> >> >> +    unsigned long flags;
>> >> >> +    u32 reg;
>> >> >> +
>> >> >> +    priv_dev = cdns->gadget_dev;
>> >> >> +    spin_lock_irqsave(&priv_dev->lock, flags);
>> >> >
>> >> >you're already running in hardirq context. Why do you need this lock
>> >> >at all? I would be better to use the hardirq handler to mask your
>> >> >interrupts, so they don't fire again, then used the top-half
>> >> >(softirq) handler to actually handle the interrupts.
>> >>
>> >
>> > This controller may be ran at SMP environment, register and flag
>> > access needs to be protected among CPUs running.
>> 
>> in hardirq context? When interrupts are already disabled?
>
> Interrupt handler (hardirq context) at CPU0, and process at CPU1, eg
> role switch, unload module, etc.

the process at CPU1 would need to disable interrupts (spin_lock_irq() or
spin_lock_irqsave()), not the hardirq on CPU0 as that already runs with
interrrupts disabled.

https://www.kernel.org/doc/html/latest/kernel-hacking/locking.html#table-of-minimum-requirements

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]