RE: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver

[Frank Li]

> -----Original Message-----
> From: Marc Zyngier <maz@xxxxxxxxxx>
> Sent: Thursday, September 8, 2022 9:52 AM
> To: Frank Li <frank.li@xxxxxxx>
> Cc: tglx@xxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx;
> s.hauer@xxxxxxxxxxxxxx; kw@xxxxxxxxx; bhelgaas@xxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Peng Fan
> <peng.fan@xxxxxxx>; Aisheng Dong <aisheng.dong@xxxxxxx>;
> jdmason@xxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; dl-linux-
> imx <linux-imx@xxxxxxx>; kishon@xxxxxx; lorenzo.pieralisi@xxxxxxx;
> ntb@xxxxxxxxxxxxxxx; lznuaa@xxxxxxxxx; imx@xxxxxxxxxxxxxxx;
> manivannan.sadhasivam@xxxxxxxxxx
> Subject: Re: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller
> driver
> 
> Caution: EXT Email
> 
> On Thu, 08 Sep 2022 15:23:53 +0100,
> Frank Li <frank.li@xxxxxxx> wrote:
> >
> > >
> > > On Wed, 07 Sep 2022 04:48:54 +0100,
> > > Frank Li <Frank.Li@xxxxxxx> wrote:
> > > >
> > > > The MU block found in a number of Freescale/NXP SoCs supports
> > > generating
> > > > IRQs by writing data to a register
> > > >
> > > > This enables the MU block to be used as a MSI controller, by leveraging
> > > > the platform-MSI API
> > >
> > > Missing full stop after each sentence.
> >
> > [Frank Li] Do you means missed "."?
> 
> Yes.
> 
> > > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > > > index 5e4e50122777d..e04c6521dce55 100644
> > > > --- a/drivers/irqchip/Kconfig
> > > > +++ b/drivers/irqchip/Kconfig
> > > > @@ -470,6 +470,15 @@ config IMX_INTMUX
> > > >       help
> > > >         Support for the i.MX INTMUX interrupt multiplexer.
> > > >
> > > > +config IMX_MU_MSI
> > > > +     bool "i.MX MU work as MSI controller"
> > >
> > > Why bool? Doesn't it also work as a module?
> >
> > [Frank Li] I remember you said that irq-chip can't be removed.
> > So I am not sure why need build as module.
> 
> Not being removed doesn't mean it cannot be built as a module and
> loaded on demand. Why should I be forced to have this driver built-in
> if my kernel is used on a variety of systems, only one of them having
> this device?
> 
> > > > +
> > > > +struct imx_mu_msi {
> > > > +     spinlock_t                      lock;
> > > > +     raw_spinlock_t                  reglock;
> > >
> > > Why two locks? Isn't one enough to protect both MSI allocation (which
> > > happens once in a blue moon) and register access?
> >
> > [Frank Li] Previously your comment, ask me to use raw_spinlock for
> > read\write register access.  I don't think raw_spinlock is good for
> > MSI allocation.
> 
> Why wouldn't it be good enough? I'd really like to know.[Frank Li] '

[Frank Li] According to my understand, raw_spinlock skip some lockdep
/debug feature to get better performance, which should be used when
Frequently call, such as irq handle\polling thread. 

Spinlock have DEBUG feature to check wrong use lock.  Allocate MSI generally
only is call once when driver probe.  

The basic principle,  lock should be used only when necessary.  Access reg and
Allocate msi is totally independence events.

For this case, it is not big detail.  

1.  change spin_lock to raw_spin_lock at allocate msi function.

2.  kept define spinlock_t lock;
     In register modify function using
	Raw_spin_lock(spinlock_check(lock), flags).

> 
> >
> > >
> > > Also, where are these locks initialised?
> > >
> >
> > [Frank Li] struct imx_mu_msi is fill zero when allocated.
> > Does it still need additional initialization for spinlock?
> 
> Have you heard of lockdep? Or CONFIG_DEBUG_SPINLOCK?  Maybe you
> should
> try it.
> 
> > > > +     if (!pdev)
> > > > +             return -ENODEV;
> > >
> > > How can that happen?
> > >
> > [Frank Li] Not sure, many driver check as it.
> 
> And? Just because someone does something pointless, you have to
> imitate them?
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.