netdev - 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]
Date:   Fri, 19 Aug 2022 01:50:47 +0000
From:   Wei Fang <wei.fang@....com>
To:     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>, Peng Fan <peng.fan@....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



> -----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 ?