| Message ID | 20220407130625.190078-1-michael@walle.cc (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: dsa: felix: suppress -EPROBE_DEFER errors | expand |
On Thu, Apr 07, 2022 at 03:06:25PM +0200, Michael Walle wrote: > Due to missing prerequisites the probe of the felix switch might be > deferred: > [ 4.435305] mscc_felix 0000:00:00.5: Failed to register DSA switch: -517 > > It's not an error. Use dev_err_probe() to demote the error to a debug > message. While at it, replace all the dev_err()'s in the probe with > dev_err_probe(). > > Signed-off-by: Michael Walle <michael@walle.cc> > --- Please limit the dev_err_probe() to dsa_register_switch(). The resource that is missing is the DSA master, see of_find_net_device_by_node(). The others cannot possibly return -EPROBE_DEFER. > > Should this be a patch with a Fixes tag? Whichever way you wish, no preference. > > drivers/net/dsa/ocelot/felix_vsc9959.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c > index 8d382b27e625..1f8c4c6de01b 100644 > --- a/drivers/net/dsa/ocelot/felix_vsc9959.c > +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c > @@ -2268,14 +2268,14 @@ static int felix_pci_probe(struct pci_dev *pdev, > > err = pci_enable_device(pdev); > if (err) { > - dev_err(&pdev->dev, "device enable failed\n"); > + dev_err_probe(&pdev->dev, err, "device enable failed\n"); > goto err_pci_enable; > } > > felix = kzalloc(sizeof(struct felix), GFP_KERNEL); > if (!felix) { > err = -ENOMEM; > - dev_err(&pdev->dev, "Failed to allocate driver memory\n"); > + dev_err_probe(&pdev->dev, err, "Failed to allocate driver memory\n"); > goto err_alloc_felix; > } > > @@ -2293,7 +2293,7 @@ static int felix_pci_probe(struct pci_dev *pdev, > &felix_irq_handler, IRQF_ONESHOT, > "felix-intb", ocelot); > if (err) { > - dev_err(&pdev->dev, "Failed to request irq\n"); > + dev_err_probe(&pdev->dev, err, "Failed to request irq\n"); > goto err_alloc_irq; > } > > @@ -2316,7 +2316,7 @@ static int felix_pci_probe(struct pci_dev *pdev, > > err = dsa_register_switch(ds); > if (err) { > - dev_err(&pdev->dev, "Failed to register DSA switch: %d\n", err); > + dev_err_probe(&pdev->dev, err, "Failed to register DSA switch\n"); > goto err_register_ds; > } > > -- > 2.30.2 >
Am 2022-04-07 15:56, schrieb Vladimir Oltean: > On Thu, Apr 07, 2022 at 03:06:25PM +0200, Michael Walle wrote: >> Due to missing prerequisites the probe of the felix switch might be >> deferred: >> [ 4.435305] mscc_felix 0000:00:00.5: Failed to register DSA switch: >> -517 >> >> It's not an error. Use dev_err_probe() to demote the error to a debug >> message. While at it, replace all the dev_err()'s in the probe with >> dev_err_probe(). >> >> Signed-off-by: Michael Walle <michael@walle.cc> >> --- > > Please limit the dev_err_probe() to dsa_register_switch(). The resource > that is missing is the DSA master, see of_find_net_device_by_node(). > The others cannot possibly return -EPROBE_DEFER. This was my rationale (from the function doc): * Note that it is deemed acceptable to use this function for error * prints during probe even if the @err is known to never be -EPROBE_DEFER. * The benefit compared to a normal dev_err() is the standardized format * of the error code and the fact that the error code is returned. In any case I don't have a strong opinion. >> >> Should this be a patch with a Fixes tag? > > Whichever way you wish, no preference. I'll limit it to just the one dev_err() and add a Fixes, there might be scripts out there who greps dmesg for errors. -michael
On Thu, Apr 07, 2022 at 04:04:20PM +0200, Michael Walle wrote: > Am 2022-04-07 15:56, schrieb Vladimir Oltean: > > On Thu, Apr 07, 2022 at 03:06:25PM +0200, Michael Walle wrote: > > > Due to missing prerequisites the probe of the felix switch might be > > > deferred: > > > [ 4.435305] mscc_felix 0000:00:00.5: Failed to register DSA > > > switch: -517 > > > > > > It's not an error. Use dev_err_probe() to demote the error to a debug > > > message. While at it, replace all the dev_err()'s in the probe with > > > dev_err_probe(). > > > > > > Signed-off-by: Michael Walle <michael@walle.cc> > > > --- > > > > Please limit the dev_err_probe() to dsa_register_switch(). The resource > > that is missing is the DSA master, see of_find_net_device_by_node(). > > The others cannot possibly return -EPROBE_DEFER. > > This was my rationale (from the function doc): > > * Note that it is deemed acceptable to use this function for error > * prints during probe even if the @err is known to never be -EPROBE_DEFER. > * The benefit compared to a normal dev_err() is the standardized format > * of the error code and the fact that the error code is returned. > > In any case I don't have a strong opinion. Take this case: err = -ENOMEM; - dev_err(&pdev->dev, "Failed to allocate driver memory\n"); + dev_err_probe(&pdev->dev, err, "Failed to allocate driver memory\n"); (1) there is no need to print ENOMEM if we say "failed to allocate memory" (2) we don't use the return value of dev_err_probe() anyway, we have actual teardown to do (pci_disable_device). (3) we _surely_ know that -ENOMEM != -EPROBE_DEFER > > > > > > > Should this be a patch with a Fixes tag? > > > > Whichever way you wish, no preference. > > I'll limit it to just the one dev_err() and add a Fixes, > there might be scripts out there who greps dmesg for errors. Ok.
Am 2022-04-07 16:12, schrieb Vladimir Oltean: > On Thu, Apr 07, 2022 at 04:04:20PM +0200, Michael Walle wrote: >> Am 2022-04-07 15:56, schrieb Vladimir Oltean: >> > On Thu, Apr 07, 2022 at 03:06:25PM +0200, Michael Walle wrote: >> > > Due to missing prerequisites the probe of the felix switch might be >> > > deferred: >> > > [ 4.435305] mscc_felix 0000:00:00.5: Failed to register DSA >> > > switch: -517 >> > > >> > > It's not an error. Use dev_err_probe() to demote the error to a debug >> > > message. While at it, replace all the dev_err()'s in the probe with >> > > dev_err_probe(). >> > > >> > > Signed-off-by: Michael Walle <michael@walle.cc> >> > > --- >> > >> > Please limit the dev_err_probe() to dsa_register_switch(). The resource >> > that is missing is the DSA master, see of_find_net_device_by_node(). >> > The others cannot possibly return -EPROBE_DEFER. >> >> This was my rationale (from the function doc): >> >> * Note that it is deemed acceptable to use this function for error >> * prints during probe even if the @err is known to never be >> -EPROBE_DEFER. >> * The benefit compared to a normal dev_err() is the standardized >> format >> * of the error code and the fact that the error code is returned. >> >> In any case I don't have a strong opinion. > > Take this case: > > err = -ENOMEM; > - dev_err(&pdev->dev, "Failed to allocate driver memory\n"); > + dev_err_probe(&pdev->dev, err, "Failed to allocate driver > memory\n"); > > (1) there is no need to print ENOMEM if we say "failed to allocate > memory" That was actually a brain fart of mine ;) But AFAIK there is no need in reporting ENOMEM errors anyway. -michael > (2) we don't use the return value of dev_err_probe() anyway, we have > actual teardown to do (pci_disable_device). > (3) we _surely_ know that -ENOMEM != -EPROBE_DEFER >> >> > > >> > > Should this be a patch with a Fixes tag? >> > >> > Whichever way you wish, no preference. >> >> I'll limit it to just the one dev_err() and add a Fixes, >> there might be scripts out there who greps dmesg for errors. > > Ok.
>> > > Should this be a patch with a Fixes tag? >> > >> > Whichever way you wish, no preference. >> >> I'll limit it to just the one dev_err() and add a Fixes, >> there might be scripts out there who greps dmesg for errors. > > Ok. Hum, it's not that easy. The issue goes back all the way to the initial commit if I didn't miss anything (56051948773e). That one was first included in 5.5, but dev_err_probe() wasn't added until 5.9. Thus will it work if I add Fixes: 56051948773e (..)? -michael
On Thu, 07 Apr 2022 16:58:43 +0200 Michael Walle wrote: > >> I'll limit it to just the one dev_err() and add a Fixes, > >> there might be scripts out there who greps dmesg for errors. > > > > Ok. > > Hum, it's not that easy. The issue goes back all the way > to the initial commit if I didn't miss anything (56051948773e). > That one was first included in 5.5, but dev_err_probe() wasn't > added until 5.9. > > Thus will it work if I add Fixes: 56051948773e (..)? Yes, backporters will figure it out. Matters even less since none of [5.5, 5.9] kernels are LTS. But if you want to slap a Fixes tag on it, it has to go to net rather than net-next.
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c index 8d382b27e625..1f8c4c6de01b 100644 --- a/drivers/net/dsa/ocelot/felix_vsc9959.c +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c @@ -2268,14 +2268,14 @@ static int felix_pci_probe(struct pci_dev *pdev, err = pci_enable_device(pdev); if (err) { - dev_err(&pdev->dev, "device enable failed\n"); + dev_err_probe(&pdev->dev, err, "device enable failed\n"); goto err_pci_enable; } felix = kzalloc(sizeof(struct felix), GFP_KERNEL); if (!felix) { err = -ENOMEM; - dev_err(&pdev->dev, "Failed to allocate driver memory\n"); + dev_err_probe(&pdev->dev, err, "Failed to allocate driver memory\n"); goto err_alloc_felix; } @@ -2293,7 +2293,7 @@ static int felix_pci_probe(struct pci_dev *pdev, &felix_irq_handler, IRQF_ONESHOT, "felix-intb", ocelot); if (err) { - dev_err(&pdev->dev, "Failed to request irq\n"); + dev_err_probe(&pdev->dev, err, "Failed to request irq\n"); goto err_alloc_irq; } @@ -2316,7 +2316,7 @@ static int felix_pci_probe(struct pci_dev *pdev, err = dsa_register_switch(ds); if (err) { - dev_err(&pdev->dev, "Failed to register DSA switch: %d\n", err); + dev_err_probe(&pdev->dev, err, "Failed to register DSA switch\n"); goto err_register_ds; }
Due to missing prerequisites the probe of the felix switch might be deferred: [ 4.435305] mscc_felix 0000:00:00.5: Failed to register DSA switch: -517 It's not an error. Use dev_err_probe() to demote the error to a debug message. While at it, replace all the dev_err()'s in the probe with dev_err_probe(). Signed-off-by: Michael Walle <michael@walle.cc> --- Should this be a patch with a Fixes tag? drivers/net/dsa/ocelot/felix_vsc9959.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)