| Message ID | 20220729130346.2961889-8-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: microchip: add error handling and register access validation | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 11 of 11 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | warning | WARNING: quoted string split across lines |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Fri, Jul 29, 2022 at 03:03:43PM +0200, Oleksij Rempel wrote: > KSZ9893 family of chips do not support synclko property. So warn about > without preventing driver from start. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/dsa/microchip/ksz_common.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c > index 71b5349d006a..d3a9836c706f 100644 > --- a/drivers/net/dsa/microchip/ksz_common.c > +++ b/drivers/net/dsa/microchip/ksz_common.c > @@ -1916,6 +1916,13 @@ int ksz_switch_register(struct ksz_device *dev) > dev_err(dev->dev, "inconsistent synclko settings\n"); > return -EINVAL; > } > + > + if (dev->chip_id == KSZ9893_CHIP_ID && (dev->synclko_125 || > + dev->synclko_disable)) { > + dev_warn(dev->dev, "microchip,synclko-125 and microchip,synclko-disable " > + "properties are not supported on this chip. " > + "Please fix you devicetree.\n"); s/you/your/ Does KSZ8 have a REFCLK output of any sort? If it doesn't, then "microchip,synclko-disable" is kind of supported, right? I wonder what there is to gain by saying that you should remove some device tree properties from non-ksz9477. After all, anyone can add any random properties to a KSZ8 switch OF node and you won't warn about those. > + } > } > > ret = dsa_register_switch(dev->ds); > -- > 2.30.2 >
On Tue, Aug 02, 2022 at 02:36:33PM +0300, Vladimir Oltean wrote: > On Fri, Jul 29, 2022 at 03:03:43PM +0200, Oleksij Rempel wrote: > > KSZ9893 family of chips do not support synclko property. So warn about > > without preventing driver from start. > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > --- > > drivers/net/dsa/microchip/ksz_common.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c > > index 71b5349d006a..d3a9836c706f 100644 > > --- a/drivers/net/dsa/microchip/ksz_common.c > > +++ b/drivers/net/dsa/microchip/ksz_common.c > > @@ -1916,6 +1916,13 @@ int ksz_switch_register(struct ksz_device *dev) > > dev_err(dev->dev, "inconsistent synclko settings\n"); > > return -EINVAL; > > } > > + > > + if (dev->chip_id == KSZ9893_CHIP_ID && (dev->synclko_125 || > > + dev->synclko_disable)) { > > + dev_warn(dev->dev, "microchip,synclko-125 and microchip,synclko-disable " > > + "properties are not supported on this chip. " > > + "Please fix you devicetree.\n"); > > s/you/your/ > > Does KSZ8 have a REFCLK output of any sort? If it doesn't, then > "microchip,synclko-disable" is kind of supported, right? > > I wonder what there is to gain by saying that you should remove some > device tree properties from non-ksz9477. After all, anyone can add any > random properties to a KSZ8 switch OF node and you won't warn about > those. Hm, if we will have any random not support OF property in the switch node. We won't be able to warn about it anyway. So, if it is present but not supported, we will just ignore it. I'll drop this patch. Regards, Oleksij
On Fri, Aug 05, 2022 at 01:56:01PM +0200, Oleksij Rempel wrote: > Hm, if we will have any random not support OF property in the switch > node. We won't be able to warn about it anyway. So, if it is present > but not supported, we will just ignore it. > > I'll drop this patch. To continue, I think the right way to go about this is to edit the dt-schema to say that these properties are only applicable to certain compatible strings, rather than for all. Then due to the "unevaluatedProperties: false", you'd get the warnings you want, at validation time.
On Fri, Aug 05, 2022 at 04:42:34PM +0300, Vladimir Oltean wrote: > On Fri, Aug 05, 2022 at 01:56:01PM +0200, Oleksij Rempel wrote: > > Hm, if we will have any random not support OF property in the switch > > node. We won't be able to warn about it anyway. So, if it is present > > but not supported, we will just ignore it. > > > > I'll drop this patch. > > To continue, I think the right way to go about this is to edit the > dt-schema to say that these properties are only applicable to certain > compatible strings, rather than for all. Then due to the > "unevaluatedProperties: false", you'd get the warnings you want, at > validation time. Hm, with "unevaluatedProperties: false" i have no warnings. Even if I create examples with random strings as properties. Are there some new json libraries i should use? Regards, Oleksij
On Sat, Aug 13, 2022 at 04:32:15PM +0200, Oleksij Rempel wrote: > On Fri, Aug 05, 2022 at 04:42:34PM +0300, Vladimir Oltean wrote: > > On Fri, Aug 05, 2022 at 01:56:01PM +0200, Oleksij Rempel wrote: > > > Hm, if we will have any random not support OF property in the switch > > > node. We won't be able to warn about it anyway. So, if it is present > > > but not supported, we will just ignore it. > > > > > > I'll drop this patch. > > > > To continue, I think the right way to go about this is to edit the > > dt-schema to say that these properties are only applicable to certain > > compatible strings, rather than for all. Then due to the > > "unevaluatedProperties: false", you'd get the warnings you want, at > > validation time. > > Hm, with "unevaluatedProperties: false" i have no warnings. Even if I > create examples with random strings as properties. Are there some new > json libraries i should use? Try additionalProperties: False Andrew
On Sat, Aug 13, 2022 at 05:11:45PM +0200, Andrew Lunn wrote: > On Sat, Aug 13, 2022 at 04:32:15PM +0200, Oleksij Rempel wrote: > > On Fri, Aug 05, 2022 at 04:42:34PM +0300, Vladimir Oltean wrote: > > > On Fri, Aug 05, 2022 at 01:56:01PM +0200, Oleksij Rempel wrote: > > > > Hm, if we will have any random not support OF property in the switch > > > > node. We won't be able to warn about it anyway. So, if it is present > > > > but not supported, we will just ignore it. > > > > > > > > I'll drop this patch. > > > > > > To continue, I think the right way to go about this is to edit the > > > dt-schema to say that these properties are only applicable to certain > > > compatible strings, rather than for all. Then due to the > > > "unevaluatedProperties: false", you'd get the warnings you want, at > > > validation time. > > > > Hm, with "unevaluatedProperties: false" i have no warnings. Even if I > > create examples with random strings as properties. Are there some new > > json libraries i should use? > > Try > > additionalProperties: False Yes, it works. But in this case I'll do more changes. Just wont to make sure I do not fix not broken things. Regards, Oleksij
On Sat, Aug 13, 2022 at 06:18:50PM +0200, Oleksij Rempel wrote: > On Sat, Aug 13, 2022 at 05:11:45PM +0200, Andrew Lunn wrote: > > On Sat, Aug 13, 2022 at 04:32:15PM +0200, Oleksij Rempel wrote: > > > On Fri, Aug 05, 2022 at 04:42:34PM +0300, Vladimir Oltean wrote: > > > > On Fri, Aug 05, 2022 at 01:56:01PM +0200, Oleksij Rempel wrote: > > > > > Hm, if we will have any random not support OF property in the switch > > > > > node. We won't be able to warn about it anyway. So, if it is present > > > > > but not supported, we will just ignore it. > > > > > > > > > > I'll drop this patch. > > > > > > > > To continue, I think the right way to go about this is to edit the > > > > dt-schema to say that these properties are only applicable to certain > > > > compatible strings, rather than for all. Then due to the > > > > "unevaluatedProperties: false", you'd get the warnings you want, at > > > > validation time. > > > > > > Hm, with "unevaluatedProperties: false" i have no warnings. Even if I > > > create examples with random strings as properties. Are there some new > > > json libraries i should use? > > > > Try > > > > additionalProperties: False > > Yes, it works. But in this case I'll do more changes. Just wont to make > sure I do not fix not broken things. I've been working on converting some old SoCs bindings from .txt to .yaml. My observations is that the yaml is sometimes more restrictive than what the drivers actually imposes. So you might need to change perfectly working .dts files to get it warning free. Or you just accept the warnings and move on. At lot will depend on the number of warnings and how easy it is to see real problems mixed in with warnings you never intend to fix. Andrew
Ccing Rob Herring. On Sat, Aug 13, 2022 at 10:42:05PM +0200, Andrew Lunn wrote: > On Sat, Aug 13, 2022 at 06:18:50PM +0200, Oleksij Rempel wrote: > > On Sat, Aug 13, 2022 at 05:11:45PM +0200, Andrew Lunn wrote: > > > On Sat, Aug 13, 2022 at 04:32:15PM +0200, Oleksij Rempel wrote: > > > > On Fri, Aug 05, 2022 at 04:42:34PM +0300, Vladimir Oltean wrote: > > > > > On Fri, Aug 05, 2022 at 01:56:01PM +0200, Oleksij Rempel wrote: > > > > > > Hm, if we will have any random not support OF property in the switch > > > > > > node. We won't be able to warn about it anyway. So, if it is present > > > > > > but not supported, we will just ignore it. > > > > > > > > > > > > I'll drop this patch. > > > > > > > > > > To continue, I think the right way to go about this is to edit the > > > > > dt-schema to say that these properties are only applicable to certain > > > > > compatible strings, rather than for all. Then due to the > > > > > "unevaluatedProperties: false", you'd get the warnings you want, at > > > > > validation time. > > > > > > > > Hm, with "unevaluatedProperties: false" i have no warnings. Even if I > > > > create examples with random strings as properties. Are there some new > > > > json libraries i should use? > > > > > > Try > > > > > > additionalProperties: False > > > > Yes, it works. But in this case I'll do more changes. Just wont to make > > sure I do not fix not broken things. > > I've been working on converting some old SoCs bindings from .txt to > .yaml. My observations is that the yaml is sometimes more restrictive > than what the drivers actually imposes. So you might need to change > perfectly working .dts files to get it warning free. Or you just > accept the warnings and move on. At lot will depend on the number of > warnings and how easy it is to see real problems mixed in with > warnings you never intend to fix. Heh :) Currently with "unevaluatedProperties: false" restrictions do not work at all. At least for me. For example with this change I have no warnings: diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml index 1e26d876d1463..da38ad98a152f 100644 --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml @@ -120,6 +120,7 @@ examples: ethernet-switch@1 { reg = <0x1>; compatible = "nxp,sja1105t"; + something-random-here; ethernet-ports { #address-cells = <1>; make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml So the main question is, is it broken for all or just for me? If it is just me, what i'm doing wrong? Regards, Oleksij
On Sun, Aug 14, 2022 at 06:26:08AM +0200, Oleksij Rempel wrote: > Heh :) Currently with "unevaluatedProperties: false" restrictions do not > work at all. At least for me. For example with this change I have no > warnings: > diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml > index 1e26d876d1463..da38ad98a152f 100644 > --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml > @@ -120,6 +120,7 @@ examples: > ethernet-switch@1 { > reg = <0x1>; > compatible = "nxp,sja1105t"; > + something-random-here; > > ethernet-ports { > #address-cells = <1>; > > make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml > > So the main question is, is it broken for all or just for me? If it is > just me, what i'm doing wrong? Might it be due to the additionalProperties: true from spi-peripheral-props.yaml?
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index 71b5349d006a..d3a9836c706f 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -1916,6 +1916,13 @@ int ksz_switch_register(struct ksz_device *dev) dev_err(dev->dev, "inconsistent synclko settings\n"); return -EINVAL; } + + if (dev->chip_id == KSZ9893_CHIP_ID && (dev->synclko_125 || + dev->synclko_disable)) { + dev_warn(dev->dev, "microchip,synclko-125 and microchip,synclko-disable " + "properties are not supported on this chip. " + "Please fix you devicetree.\n"); + } } ret = dsa_register_switch(dev->ds);
KSZ9893 family of chips do not support synclko property. So warn about without preventing driver from start. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/dsa/microchip/ksz_common.c | 7 +++++++ 1 file changed, 7 insertions(+)