| Message ID | 20220608120358.81147-2-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ptp_ocp: set of small cleanups | expand |
On 08.06.2022 13:03, Andy Shevchenko wrote: > Simplify the error path in ->probe() and unify message format a bit > by using dev_err_probe(). > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> LGTM Acked-by: Vadim Fedorenko <vfedorenko@novek.ru> > --- > drivers/ptp/ptp_ocp.c | 13 +++++-------- > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c > index 4519ef42b458..17930762fde9 100644 > --- a/drivers/ptp/ptp_ocp.c > +++ b/drivers/ptp/ptp_ocp.c > @@ -3722,14 +3722,12 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) > int err; > > devlink = devlink_alloc(&ptp_ocp_devlink_ops, sizeof(*bp), &pdev->dev); > - if (!devlink) { > - dev_err(&pdev->dev, "devlink_alloc failed\n"); > - return -ENOMEM; > - } > + if (!devlink) > + return dev_err_probe(&pdev->dev, -ENOMEM, "devlink_alloc failed\n"); > > err = pci_enable_device(pdev); > if (err) { > - dev_err(&pdev->dev, "pci_enable_device\n"); > + dev_err_probe(&pdev->dev, err, "pci_enable_device\n"); > goto out_free; > } > > @@ -3745,7 +3743,7 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) > */ > err = pci_alloc_irq_vectors(pdev, 1, 17, PCI_IRQ_MSI | PCI_IRQ_MSIX); > if (err < 0) { > - dev_err(&pdev->dev, "alloc_irq_vectors err: %d\n", err); > + dev_err_probe(&pdev->dev, err, "alloc_irq_vectors\n"); > goto out; > } > bp->n_irqs = err; > @@ -3757,8 +3755,7 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > bp->ptp = ptp_clock_register(&bp->ptp_info, &pdev->dev); > if (IS_ERR(bp->ptp)) { > - err = PTR_ERR(bp->ptp); > - dev_err(&pdev->dev, "ptp_clock_register: %d\n", err); > + err = dev_err_probe(&pdev->dev, PTR_ERR(bp->ptp), "ptp_clock_register\n"); > bp->ptp = NULL; > goto out; > }
On Wed, Jun 08, 2022 at 03:03:54PM +0300, Andy Shevchenko wrote: > Simplify the error path in ->probe() and unify message format a bit > by using dev_err_probe(). Line length.
On Wed, 8 Jun 2022 15:03:54 +0300 Andy Shevchenko wrote: > Simplify the error path in ->probe() and unify message format a bit > by using dev_err_probe(). Let's not do this. I get that using dev_err_probe() even without possibility of -EPROBE_DEFER is acceptable, but converting existing drivers is too much IMO. Acceptable < best practice.
On Thu, Jun 09, 2022 at 10:45:23PM -0700, Jakub Kicinski wrote: > On Wed, 8 Jun 2022 15:03:54 +0300 Andy Shevchenko wrote: > > Simplify the error path in ->probe() and unify message format a bit > > by using dev_err_probe(). > > Let's not do this. I get that using dev_err_probe() even without > possibility of -EPROBE_DEFER is acceptable, but converting > existing drivers is too much IMO. Acceptable < best practice. Noted. I have just checked that if you drop this patch the rest will be still applicable. If you have no objections, can you apply patches 2-5 then?
On Fri, 10 Jun 2022 14:09:24 +0300 Andy Shevchenko wrote: > I have just checked that if you drop this patch the rest will be still > applicable. If you have no objections, can you apply patches 2-5 then? It's tradition in netdev to ask people to repost. But looks completely safe for me to drop patch 1, so applied 2-5. Don't tell anyone I did this.
On Fri, Jun 10, 2022 at 08:39:18AM -0700, Jakub Kicinski wrote: > On Fri, 10 Jun 2022 14:09:24 +0300 Andy Shevchenko wrote: > > I have just checked that if you drop this patch the rest will be still > > applicable. If you have no objections, can you apply patches 2-5 then? > > It's tradition in netdev to ask people to repost. But looks completely > safe for me to drop patch 1, so applied 2-5. Don't tell anyone I did this. I see what you did there. :)
On Fri, Jun 10, 2022 at 5:43 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 10 Jun 2022 14:09:24 +0300 Andy Shevchenko wrote: > > I have just checked that if you drop this patch the rest will be still > > applicable. If you have no objections, can you apply patches 2-5 then? > > It's tradition in netdev to ask people to repost. But looks completely > safe for me to drop patch 1, so applied 2-5. Thanks! > Don't tell anyone I did this. Tschhh!
diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c index 4519ef42b458..17930762fde9 100644 --- a/drivers/ptp/ptp_ocp.c +++ b/drivers/ptp/ptp_ocp.c @@ -3722,14 +3722,12 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) int err; devlink = devlink_alloc(&ptp_ocp_devlink_ops, sizeof(*bp), &pdev->dev); - if (!devlink) { - dev_err(&pdev->dev, "devlink_alloc failed\n"); - return -ENOMEM; - } + if (!devlink) + return dev_err_probe(&pdev->dev, -ENOMEM, "devlink_alloc failed\n"); err = pci_enable_device(pdev); if (err) { - dev_err(&pdev->dev, "pci_enable_device\n"); + dev_err_probe(&pdev->dev, err, "pci_enable_device\n"); goto out_free; } @@ -3745,7 +3743,7 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) */ err = pci_alloc_irq_vectors(pdev, 1, 17, PCI_IRQ_MSI | PCI_IRQ_MSIX); if (err < 0) { - dev_err(&pdev->dev, "alloc_irq_vectors err: %d\n", err); + dev_err_probe(&pdev->dev, err, "alloc_irq_vectors\n"); goto out; } bp->n_irqs = err; @@ -3757,8 +3755,7 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) bp->ptp = ptp_clock_register(&bp->ptp_info, &pdev->dev); if (IS_ERR(bp->ptp)) { - err = PTR_ERR(bp->ptp); - dev_err(&pdev->dev, "ptp_clock_register: %d\n", err); + err = dev_err_probe(&pdev->dev, PTR_ERR(bp->ptp), "ptp_clock_register\n"); bp->ptp = NULL; goto out; }
Simplify the error path in ->probe() and unify message format a bit by using dev_err_probe(). Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/ptp/ptp_ocp.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)