| Message ID | 20200312164320.22349-1-michael@walle.cc |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/2] net: dsa: felix: allow the device to be disabled | expand |
On Thu, 12 Mar 2020 at 18:44, Michael Walle <michael@walle.cc> wrote: > > If there is no specific configuration of the felix switch in the device > tree, but only the default configuration (ie. given by the SoCs dtsi > file), the probe fails because no CPU port has been set. On the other > hand you cannot set a default CPU port because that depends on the > actual board using the switch. > > [ 2.701300] DSA: tree 0 has no CPU port > [ 2.705167] mscc_felix 0000:00:00.5: Failed to register DSA switch: -22 > [ 2.711844] mscc_felix: probe of 0000:00:00.5 failed with error -22 > > Thus let the device tree disable this device entirely, like it is also > done with the enetc driver of the same SoC. > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > drivers/net/dsa/ocelot/felix.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c > index 69546383a382..531c7710063f 100644 > --- a/drivers/net/dsa/ocelot/felix.c > +++ b/drivers/net/dsa/ocelot/felix.c > @@ -699,6 +699,11 @@ static int felix_pci_probe(struct pci_dev *pdev, > struct felix *felix; > int err; > > + if (pdev->dev.of_node && !of_device_is_available(pdev->dev.of_node)) { > + dev_info(&pdev->dev, "device is disabled, skipping\n"); > + return -ENODEV; > + } > + IMHO since DSA is already dependent on device tree for PHY bindings, it would make more sense to move this there: diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index e7c30b472034..f7ca01d93928 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -878,7 +878,7 @@ static int dsa_switch_probe(struct dsa_switch *ds) if (!ds->num_ports) return -EINVAL; - if (np) { + if (np && of_device_is_available(np)) { err = dsa_switch_parse_of(ds, np); if (err) dsa_switch_release_ports(ds); so that we could enforce more uniform behavior across device drivers. Then you might want to make felix shut up: diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c index 35124ef7e75b..fbd17fa94bff 100644 --- a/drivers/net/dsa/ocelot/felix.c +++ b/drivers/net/dsa/ocelot/felix.c @@ -712,10 +712,8 @@ static int felix_pci_probe(struct pci_dev *pdev, felix->ds = ds; err = dsa_register_switch(ds); - if (err) { - dev_err(&pdev->dev, "Failed to register DSA switch: %d\n", err); + if (err) goto err_register_ds; - } return 0; This has the disadvantage of not printing the "nice" "device is disabled, skipping" message (useless in my opinion), but the advantage of also shutting up on -EPROBE_DEFER. > err = pci_enable_device(pdev); > if (err) { > dev_err(&pdev->dev, "device enable failed\n"); > -- > 2.20.1 > Thanks, -Vladimir
On 3/12/20 2:35 PM, Vladimir Oltean wrote: > On Thu, 12 Mar 2020 at 18:44, Michael Walle <michael@walle.cc> wrote: >> >> If there is no specific configuration of the felix switch in the device >> tree, but only the default configuration (ie. given by the SoCs dtsi >> file), the probe fails because no CPU port has been set. On the other >> hand you cannot set a default CPU port because that depends on the >> actual board using the switch. >> >> [ 2.701300] DSA: tree 0 has no CPU port >> [ 2.705167] mscc_felix 0000:00:00.5: Failed to register DSA switch: -22 >> [ 2.711844] mscc_felix: probe of 0000:00:00.5 failed with error -22 >> >> Thus let the device tree disable this device entirely, like it is also >> done with the enetc driver of the same SoC. >> >> Signed-off-by: Michael Walle <michael@walle.cc> >> --- >> drivers/net/dsa/ocelot/felix.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c >> index 69546383a382..531c7710063f 100644 >> --- a/drivers/net/dsa/ocelot/felix.c >> +++ b/drivers/net/dsa/ocelot/felix.c >> @@ -699,6 +699,11 @@ static int felix_pci_probe(struct pci_dev *pdev, >> struct felix *felix; >> int err; >> >> + if (pdev->dev.of_node && !of_device_is_available(pdev->dev.of_node)) { >> + dev_info(&pdev->dev, "device is disabled, skipping\n"); >> + return -ENODEV; >> + } >> + > > IMHO since DSA is already dependent on device tree for PHY bindings, > it would make more sense to move this there: Michael's solution makes more sense, as this is a driver specific problem whereby you have a pci_dev instance that is created and does not honor the status property provided in Device Tree. If you were to look for a proper solution it would likely be within the PCI core actually. No other DSA switch has that problem because they use the I2C/SPI/platform_device/GPIO/whatever entry points into the driver model.
On Thu, 12 Mar 2020 at 23:45, Florian Fainelli <f.fainelli@gmail.com> wrote: > > On 3/12/20 2:35 PM, Vladimir Oltean wrote: > > On Thu, 12 Mar 2020 at 18:44, Michael Walle <michael@walle.cc> wrote: > >> > >> If there is no specific configuration of the felix switch in the device > >> tree, but only the default configuration (ie. given by the SoCs dtsi > >> file), the probe fails because no CPU port has been set. On the other > >> hand you cannot set a default CPU port because that depends on the > >> actual board using the switch. > >> > >> [ 2.701300] DSA: tree 0 has no CPU port > >> [ 2.705167] mscc_felix 0000:00:00.5: Failed to register DSA switch: -22 > >> [ 2.711844] mscc_felix: probe of 0000:00:00.5 failed with error -22 > >> > >> Thus let the device tree disable this device entirely, like it is also > >> done with the enetc driver of the same SoC. > >> > >> Signed-off-by: Michael Walle <michael@walle.cc> > >> --- > >> drivers/net/dsa/ocelot/felix.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c > >> index 69546383a382..531c7710063f 100644 > >> --- a/drivers/net/dsa/ocelot/felix.c > >> +++ b/drivers/net/dsa/ocelot/felix.c > >> @@ -699,6 +699,11 @@ static int felix_pci_probe(struct pci_dev *pdev, > >> struct felix *felix; > >> int err; > >> > >> + if (pdev->dev.of_node && !of_device_is_available(pdev->dev.of_node)) { > >> + dev_info(&pdev->dev, "device is disabled, skipping\n"); > >> + return -ENODEV; > >> + } > >> + > > > > IMHO since DSA is already dependent on device tree for PHY bindings, > > it would make more sense to move this there: > > Michael's solution makes more sense, as this is a driver specific > problem whereby you have a pci_dev instance that is created and does not > honor the status property provided in Device Tree. If you were to look > for a proper solution it would likely be within the PCI core actually. > > No other DSA switch has that problem because they use the > I2C/SPI/platform_device/GPIO/whatever entry points into the driver model. True, my problem with doing it in the PCI core is that "the book" [0] doesn't actually say anything about the "status" property, so this patch might get some pushback from the PCI maintainers (but I don't actually know): diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 512cb4312ddd..50c2b3da134a 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2281,6 +2281,12 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn) pci_set_of_node(dev); + if (dev->dev.of_node && !of_device_is_available(dev->dev.of_node)) { + pci_bus_put(dev->bus); + kfree(dev); + return NULL; + } + if (pci_setup_device(dev)) { pci_bus_put(dev->bus); kfree(dev); OF bindings are completely optional with PCI, as you probably already know. But there's nothing "driver" specific in disabling (i.e. not probing) a device. > -- > [0] https://www.openfirmware.info/data/docs/bus.pci.pdf Thanks, -Vladimir
e> IMHO since DSA is already dependent on device tree for PHY bindings,
> it would make more sense to move this there:
That is not really true. You can instantiate a marvell switch using a
platform device. So long any you only have C22 PHYs in a sane
configuration, it will just work. There are boards out there do this,
on x86 platforms without device tree.
Andrew
This series depends upon some devicetree tree changes, so why don't you
submit these changes there and add my:
Acked-by: David S. Miller <davem@davemloft.net>
Thank you.
Hi David, Hi Shawnguo, Am 2020-03-15 04:53, schrieb David Miller: > This series depends upon some devicetree tree changes, so why don't you > submit these changes there and add my: > > Acked-by: David S. Miller <davem@davemloft.net> > > Thank you. Patch 2/2 is already in linux-next, picked by Shawnguo. Who will pick 1/2? I guess it doesn't matter through which tree it will go. -michael
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c index 69546383a382..531c7710063f 100644 --- a/drivers/net/dsa/ocelot/felix.c +++ b/drivers/net/dsa/ocelot/felix.c @@ -699,6 +699,11 @@ static int felix_pci_probe(struct pci_dev *pdev, struct felix *felix; int err; + if (pdev->dev.of_node && !of_device_is_available(pdev->dev.of_node)) { + dev_info(&pdev->dev, "device is disabled, skipping\n"); + return -ENODEV; + } + err = pci_enable_device(pdev); if (err) { dev_err(&pdev->dev, "device enable failed\n");
If there is no specific configuration of the felix switch in the device tree, but only the default configuration (ie. given by the SoCs dtsi file), the probe fails because no CPU port has been set. On the other hand you cannot set a default CPU port because that depends on the actual board using the switch. [ 2.701300] DSA: tree 0 has no CPU port [ 2.705167] mscc_felix 0000:00:00.5: Failed to register DSA switch: -22 [ 2.711844] mscc_felix: probe of 0000:00:00.5 failed with error -22 Thus let the device tree disable this device entirely, like it is also done with the enetc driver of the same SoC. Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/net/dsa/ocelot/felix.c | 5 +++++ 1 file changed, 5 insertions(+)