RE: [PATCH 1/2] net: fec: Propagate PTP initialization errors

[Wei Fang]

> > > +     if (irq > 0) {
> > >               ret = devm_request_irq(&pdev->dev, irq,
> fec_pps_interrupt,
> > >                                      0, pdev->name, ndev);
> > >               if (ret < 0)
> > > -                     dev_warn(&pdev->dev, "request for pps irq
> failed(%d)\n",
> > > -                              ret);
> >
> > Why remove the diagnostic log?
> >
> 
> The reason is that the error is already logged deeper in the call chain,
> after devm_request_irq() goes through several layers, with sufficient details:
> 
> -----------------------------------------------------------------------------------------
> return dev_err_probe(dev, rc, "request_irq(%u) %ps %ps %s\n",
>                                     irq, handler, thread_fn, devname ? :
> "");
> -----------------------------------------------------------------------------------------
> 
> Therefore, I removed the diagnostic in this function to avoid duplicate error
> messages.
> If you prefer, I can keep it.

That makes sense, you'd better add a description for removing the log in the
commit message. :)

> >
> > Additionally, the second patch can be merged with this patch. There is no
> > need to use two separate patches.
> >
> 
> The error propagation part in the second patch is directly related to
> the first patch :
> 
> ------------------------------------------------------------
> -       if (fep->bufdesc_ex)
> -               fec_ptp_init(pdev, irq_cnt);
> +       if (fep->bufdesc_ex) {
> +               ret = fec_ptp_init(pdev, irq_cnt);
> +               if (ret)
> +                       goto failed_reset;
> +       }
> ------------------------------------------------------------
> 
> So I agree that this part can be merged into the first patch.

Yes, this patch and the above part should be in a patch. And it should
target to net-next tree.

> However, the change in failed_init is an independent pre-existing bug:

You should add a separate patch to fix the pre-existing issue, and the
patch targets to net tree. Patches to net tree are welcome at any time.
And you need to add the target tree in the subject so that netdev
maintainers know which git tree the patch should be applied to. For
example:
    [PATCH net]        -> net tree
    [PATCH net-next]    -> net-next tree

See: https://elixir.bootlin.com/linux/v7.2/source/Documentation/process/maintainer-netdev.rst#L61

> 
> -------------------------------------------------
>  failed_init:
> -       fec_ptp_stop(pdev);
> +       if (fep->bufdesc_ex)
> +               fec_ptp_stop(pdev);
> -------------------------------------------------
> 
> Therefore, I think it would be better to keep this part as a separate patch so
> that the Fixes: tag can correctly identify the commit that introduced the bug.
> 
> What do you think?

I do not see the Fixes tag in the second patch, please add the Fixes tag and
send a separate patch to the net tree.