Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Frank Li <frank.li@nxp.com>
To:     Marc Zyngier <maz@kernel.org>
CC:     "tglx@linutronix.de" <tglx@linutronix.de>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "krzysztof.kozlowski+dt@linaro.org" 
        <krzysztof.kozlowski+dt@linaro.org>,
        "shawnguo@kernel.org" <shawnguo@kernel.org>,
        "s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
        "kw@linux.com" <kw@linux.com>,
        "bhelgaas@google.com" <bhelgaas@google.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        Peng Fan <peng.fan@nxp.com>,
        Aisheng Dong <aisheng.dong@nxp.com>,
        "jdmason@kudzu.us" <jdmason@kudzu.us>,
        "kernel@pengutronix.de" <kernel@pengutronix.de>,
        "festevam@gmail.com" <festevam@gmail.com>,
        dl-linux-imx <linux-imx@nxp.com>,
        "kishon@ti.com" <kishon@ti.com>,
        "lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
        "ntb@lists.linux.dev" <ntb@lists.linux.dev>,
        "lznuaa@gmail.com" <lznuaa@gmail.com>,
        "imx@lists.linux.dev" <imx@lists.linux.dev>,
        "manivannan.sadhasivam@linaro.org" <manivannan.sadhasivam@linaro.org>
Subject: RE: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller
 driver
Thread-Topic: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller
 driver
Thread-Index: AQHYwmzT0d5rWoxlpEiRJzxdVpfwVa3VJ32AgABtNnCAAAtoAIAAAWig
Date:   Thu, 8 Sep 2022 15:35:20 +0000
Message-ID: <AM9PR04MB8793CE5AAAB281CE628845AC88409@AM9PR04MB8793.eurprd04.prod.outlook.com>
References: <20220907034856.3101570-1-Frank.Li@nxp.com>
        <20220907034856.3101570-3-Frank.Li@nxp.com>     <87fsh2qpq4.wl-maz@kernel.org>
        <AM9PR04MB879338D6D4B55A74CD002E6D88409@AM9PR04MB8793.eurprd04.prod.outlook.com>
 <877d2dvs0d.wl-maz@kernel.org>
In-Reply-To: <877d2dvs0d.wl-maz@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?cB+NIIiM2rGnPRgXCkhJkXzvc5NuYd1HXB3fQLbGohLgwSnTzZqETahrL+o7?=
 =?us-ascii?Q?33HDAfWVd/O9OqCl2d/CapiauOhXCbbo/0f/LPdHgXONTnO1LD+/VcAfG6Zn?=
 =?us-ascii?Q?2913dFARvWZjNZc7zO5HSa/DJbWEJIIxH0D17yZiQMcLgv4EcI+fVUAl4hj6?=
 =?us-ascii?Q?4uflq+y/H3n2jT0nUpAzHqA5QnAvnpLJhA0vfCFrqQJBYO3iTtaGhLMmJQQv?=
 =?us-ascii?Q?qoAqwS5CBQbaF8mYhNAiyMNB+x9dmHJ9MOx0at7MjcmV3bK4DO6Y/FisOJr4?=
 =?us-ascii?Q?dSQ7lStuhVPHbNtN8lhdYk+GeFhWEPORsEF5dZIpMhhC4oUpZSZEepVLE7zI?=
 =?us-ascii?Q?65PvdB8HzTqZfljBrntDAnH0Ah5se3YoK1YbVhHUAYxM1Xy3vu1DKpR1PGGh?=
 =?us-ascii?Q?ZcR1f2WQ/qRSQw+T2ij4cH2L3eyhbcL/4Y+0xlGOhgXHidnvFaI55SB3fgtX?=
 =?us-ascii?Q?G6JzikLNTJQE6oQFgI1TlKMnSoP711mExdCSPnrlTibpfE/c+js4sFPBurxD?=
 =?us-ascii?Q?ghHPrK87+BbyXnWeU/F9XFMgFhaUuC/vTmcXN7YOzua/JmjLMHwQKV0sGmmM?=
 =?us-ascii?Q?mIUCrw8bUWR3Zx2QzJ9OpspL1eV1CoM8l0hckZUmKZ7EowrcfkCNXXFJa70w?=
 =?us-ascii?Q?wxC2BvDDqOJYlG4q/yq7JN+roMpRsXEppaunhJcFLwA8g3bKC5xxZ8bsPvpT?=
 =?us-ascii?Q?/rwCuZjVuoHgWRSo8o2olEtNBqmBztfK2pnGvFcCLUW7z5uXIsg1NL7BfNpL?=
 =?us-ascii?Q?wSdyLSLF0/xPrv8e1+oeiExYsdKzHus+wPdT3EcFwwvyG2DxIvxU7p1jvC1A?=
 =?us-ascii?Q?o3djBQDci46ql91DrcoylqiN9NNd/+sHtKtQF/Dn63kohdu9sMyr5nSfS99J?=
 =?us-ascii?Q?zGUWSTc8yMfIATcIlZ5WbgR+TTHdFOX4WgFTgWh+Rsr1n2V0Ux3qLrnu87aa?=
 =?us-ascii?Q?SpTCxbjonBO0NEuVaDVO0yrgC/DUMqxIRr6+IaacYk/hwYFUpfCpMRHC4sSM?=
 =?us-ascii?Q?9o6dEU2i1EY445KnhFNrQCHg9elnaX6hYdPk/b5uhyHgMMMOrJGc0nf3U/Ng?=
 =?us-ascii?Q?c9UZ1Ym9H63B5Y/2uYhIEDrbDWAaKEv471j6WbRY2baU3L1L0e7SECmuroiM?=
 =?us-ascii?Q?7nGDPuxASFM5FkmxhTFJ0owzc1tL9Nrj+MUV03eGEaEJk7uU3LeXMuwbLQ2C?=
 =?us-ascii?Q?jDvQNMQBkbsdXu/fP21irP2jOU6wZKaVpluKkXSIXWlAogUDWm3NwV//LcI4?=
 =?us-ascii?Q?A/6BGDA1OjaryQh2B80JXeHHVwnwBv+wZt5A3uYlpiMtHnd2W7iApL6h62MM?=
 =?us-ascii?Q?yc3uoAVkFFgPgSP/NfExcZwUB/oKgQsILwDefnbSyAgRZPdlGfgT5SMvOjAB?=
 =?us-ascii?Q?IwwNpSicoTQwyFkumZLmWZl/x+OspRs3I+AefkfXQwt5zfljEQB69ooKmGFm?=
 =?us-ascii?Q?PQapuSQDw4mO34AOKNYIQofkT/ukosB9IrsYHjt/9iU1AZAUZAdqKRhBw/jz?=
 =?us-ascii?Q?uOSKamERjeE7PRyPYyQyNPDfDfNcOqBicftKg23DUZfrRJSySxaRU27CvNKi?=
 =?us-ascii?Q?s3noV9i3Ao+y8KgSaz4=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: AM9PR04MB8793.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fd29796d-ea1c-4c03-ed60-08da91afbd5d
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Sep 2022 15:35:20.8231
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: ZGae8Oy0IcUeLNBs6817A5X8BvpVMxYGcPhdRLw/5Jw/GcSTc8C2kvOXcA3bCibv9F+hUR0m6IUii7D3iNdTMQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR0402MB3585
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Thursday, September 8, 2022 9:52 AM
> To: Frank Li <frank.li@nxp.com>
> Cc: tglx@linutronix.de; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kw@linux.com; bhelgaas@google.com; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-pci@vger.kernel.org; Peng Fan
> <peng.fan@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>;
> jdmason@kudzu.us; kernel@pengutronix.de; festevam@gmail.com; dl-linux-
> imx <linux-imx@nxp.com>; kishon@ti.com; lorenzo.pieralisi@arm.com;
> ntb@lists.linux.dev; lznuaa@gmail.com; imx@lists.linux.dev;
> manivannan.sadhasivam@linaro.org
> Subject: Re: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller
> driver
>=20
> Caution: EXT Email
>=20
> On Thu, 08 Sep 2022 15:23:53 +0100,
> Frank Li <frank.li@nxp.com> wrote:
> >
> > >
> > > On Wed, 07 Sep 2022 04:48:54 +0100,
> > > Frank Li <Frank.Li@nxp.com> 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 levera=
ging
> > > > the platform-MSI API
> > >
> > > Missing full stop after each sentence.
> >
> > [Frank Li] Do you means missed "."?
>=20
> Yes.
>=20
> > > > 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.
>=20
> 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?
>=20
> > > > +
> > > > +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.
>=20
> 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.=20

Spinlock have DEBUG feature to check wrong use lock.  Allocate MSI generall=
y
only is call once when driver probe. =20

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. =20

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).

>=20
> >
> > >
> > > 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?
>=20
> Have you heard of lockdep? Or CONFIG_DEBUG_SPINLOCK?  Maybe you
> should
> try it.
>=20
> > > > +     if (!pdev)
> > > > +             return -ENODEV;
> > >
> > > How can that happen?
> > >
> > [Frank Li] Not sure, many driver check as it.
>=20
> And? Just because someone does something pointless, you have to
> imitate them?
>=20
>         M.
>=20
> --
> Without deviation from the norm, progress is not possible.