linux-kernel - RE: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <PA4PR04MB9416C0E26B13D4060C2F50A9886C9@PA4PR04MB9416.eurprd04.prod.outlook.com>
Date:   Fri, 19 Aug 2022 03:13:21 +0000
From:   Peng Fan <peng.fan@....com>
To:     Wei Fang <wei.fang@....com>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Shawn Guo <shawnguo@...nel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        dl-linux-imx <linux-imx@....com>, Jacky Bai <ping.bai@....com>,
        "sudeep.holla@....com" <sudeep.holla@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Aisheng Dong <aisheng.dong@....com>
Subject: RE: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

> Subject: RE: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item
> 
> 
> 
> > -----Original Message-----
> > From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> > Sent: 2022年8月18日 22:04
> > To: Shawn Guo <shawnguo@...nel.org>
> > Cc: Wei Fang <wei.fang@....com>; davem@...emloft.net;
> > edumazet@...gle.com; kuba@...nel.org; pabeni@...hat.com;
> > robh+dt@...nel.org; krzysztof.kozlowski+dt@...aro.org;
> > s.hauer@...gutronix.de; netdev@...r.kernel.org;
> > devicetree@...r.kernel.org; linux-kernel@...r.kernel.org;
> > kernel@...gutronix.de; festevam@...il.com; dl-linux-imx
> > <linux-imx@....com>; Peng Fan <peng.fan@....com>; Jacky Bai
> > <ping.bai@....com>; sudeep.holla@....com;
> > linux-arm-kernel@...ts.infradead.org; Aisheng Dong
> > <aisheng.dong@....com>
> > Subject: Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible
> > item
> >
> > On 18/08/2022 16:57, Shawn Guo wrote:
> > > On Thu, Aug 18, 2022 at 12:46:33PM +0300, Krzysztof Kozlowski wrote:
> > >> On 18/08/2022 12:22, Shawn Guo wrote:
> > >>> On Thu, Aug 18, 2022 at 10:51:02AM +0300, Krzysztof Kozlowski wrote:
> > >>>> On 18/08/2022 04:33, Shawn Guo wrote:
> > >>>>> On Mon, Jul 04, 2022 at 11:12:09AM +0200, Krzysztof Kozlowski
> wrote:
> > >>>>>>> diff --git
> > >>>>>>> a/Documentation/devicetree/bindings/net/fsl,fec.yaml
> > >>>>>>> b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> > >>>>>>> index daa2f79a294f..6642c246951b 100644
> > >>>>>>> --- a/Documentation/devicetree/bindings/net/fsl,fec.yaml
> > >>>>>>> +++ b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> > >>>>>>> @@ -40,6 +40,10 @@ properties:
> > >>>>>>>            - enum:
> > >>>>>>>                - fsl,imx7d-fec
> > >>>>>>>            - const: fsl,imx6sx-fec
> > >>>>>>> +      - items:
> > >>>>>>> +          - enum:
> > >>>>>>> +              - fsl,imx8ulp-fec
> > >>>>>>> +          - const: fsl,imx6ul-fec
> > >>>>>>
> > >>>>>> This is wrong.  fsl,imx6ul-fec has to be followed by
> > >>>>>> fsl,imx6q-fec. I think someone made similar mistakes earlier so
> > >>>>>> this is a
> > mess.
> > >>>>>
> > >>>>> Hmm, not sure I follow this.  Supposing we want to have the
> > >>>>> following compatible for i.MX8ULP FEC, why do we have to have
> > "fsl,imx6q-fec"
> > >>>>> here?
> > >>>>>
> > >>>>> 	fec: ethernet@...50000 {
> > >>>>> 		compatible = "fsl,imx8ulp-fec", "fsl,imx6ul-fec";
> > >>>>> 		...
> > >>>>> 	};
> > >>>>
> > >>>> Because a bit earlier this bindings is saying that fsl,imx6ul-fec
> > >>>> must be followed by fsl,imx6q-fec.
> > >>>
> > >>> The FEC driver OF match table suggests that fsl,imx6ul-fec and
> > >>> fsl,imx6q-fec are not really compatible.
> > >>>
> > >>> static const struct of_device_id fec_dt_ids[] = {
> > >>>         { .compatible = "fsl,imx25-fec", .data =
> > &fec_devtype[IMX25_FEC], },
> > >>>         { .compatible = "fsl,imx27-fec", .data =
> > &fec_devtype[IMX27_FEC], },
> > >>>         { .compatible = "fsl,imx28-fec", .data =
> > &fec_devtype[IMX28_FEC], },
> > >>>         { .compatible = "fsl,imx6q-fec", .data =
> > &fec_devtype[IMX6Q_FEC], },
> > >>>         { .compatible = "fsl,mvf600-fec", .data =
> > &fec_devtype[MVF600_FEC], },
> > >>>         { .compatible = "fsl,imx6sx-fec", .data =
> > &fec_devtype[IMX6SX_FEC], },
> > >>>         { .compatible = "fsl,imx6ul-fec", .data =
> > >>> &fec_devtype[IMX6UL_FEC], },
> > >>
> > >> I don't see here any incompatibility. Binding driver with different
> > >> driver data is not a proof of incompatible devices.
> > >
> > > To me, different driver data is a good sign of incompatibility.  It
> > > mostly means that software needs to program the hardware block
> > > differently.
> >
> > Any device being 100% compatible with old one and having additional
> > features will have different driver data... so no, it's not a proof.
> > There are many of such examples and we call them compatible, because
> > the device could operate when bound by the fallback compatible.
> >
> > If this is the case here - how do I know? I raised and the answer was
> > affirmative...
> >
> > >
> > >
> > >> Additionally, the
> > >> binding describes the hardware, not the driver.
> > >>
> > >>>         { .compatible = "fsl,imx8mq-fec", .data =
> > &fec_devtype[IMX8MQ_FEC], },
> > >>>         { .compatible = "fsl,imx8qm-fec", .data =
> > &fec_devtype[IMX8QM_FEC], },
> > >>>         { /* sentinel */ }
> > >>> };
> > >>> MODULE_DEVICE_TABLE(of, fec_dt_ids);
> > >>>
> > >>> Should we fix the binding doc?
> > >>
> > >> Maybe, I don't know. The binding describes the hardware, so based
> > >> on it the devices are compatible. Changing this, except ABI impact,
> > >> would be possible with proper reason, but not based on Linux driver
> code.
> > >
> > > Well, if Linux driver code is written in the way that hardware
> > > requires, I guess that's just based on hardware characteristics.
> > >
> > > To me, having a device compatible to two devices that require
> > > different programming model is unnecessary and confusing.
> >
> > It's the first time anyone mentions here the programming model is
> > different... If it is different, the devices are likely not compatible.
> >
> > However when I raised this issue last time, there were no concerns
> > with calling them all compatible. Therefore I wonder if the folks
> > working on this driver actually know what's there... I don't know, I
> > gave recommendations based on what is described in the binding and
> > expect the engineer to come with that knowledge.
> >
> >
> Sorry, I did not explain clearly last time, I just mentioned that imx8ulp fec
> was totally reused from imx6ul and was a little different from imx6q.
> So what should I do next? Should I fix the binding doc ?

Just my understanding, saying i.MX6Q supports feature A,
i.MX6UL support feature A + B, Then i.MX6UL is compatible with i.MX6Q.

If upper is true from hardware level, then i.MX8ULP FEC node
should contain 8ulp, 6ul, 6q.

But the list may expand too long if more and more devices are supported
and hardware backward compatible

Regards,
Peng.