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

Hi Wei Fang,

Thank you for your review.

> > +     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.

> >       if (IS_ERR(fep->ptp_clock)) {
> >               fep->ptp_clock = NULL;
> >               dev_err(&pdev->dev, "ptp_clock_register failed\n");
> > +             return PTR_ERR(fep->ptp_clock);
>
> fep->ptp_clock is set to NULL on the line immediately before
> PTR_ERR(fep->ptp_clock) is evaluated, so PTR_ERR receives NULL rather
> than the original ERR_PTR value.
>

You're right. This was an oversight on my part.
The error value should be saved before clearing fep->ptp_clock:

---------------------------------------------------------------------------
if (IS_ERR(fep->ptp_clock)) {
        ret = PTR_ERR(fep->ptp_clock);
        fep->ptp_clock = NULL;
        dev_err(&pdev->dev, "ptp_clock_register failed\n");
        return ret;
}
--------------------------------------------------------------------------

>
> 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.
However, the change in failed_init is an independent pre-existing bug:

-------------------------------------------------
 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?

Best regards,
Phuc