Re: [PATCH] pch_gbe: Add NULL check for ptp_pdev in pch_gbe_probe()

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 23/07/2025 17:29, Chenyuan Yang wrote:
> On Wed, Jul 23, 2025 at 2:37 AM Vadim Fedorenko
> <vadim.fedorenko@linux.dev> wrote:
>>
>> On 23/07/2025 04:41, Chenyuan Yang wrote:
>>> Since pci_get_domain_bus_and_slot() can return NULL for PCI_DEVFN(12, 4),
>>> add NULL check for adapter->ptp_pdev in pch_gbe_probe().
>>>
>>> This change is similar to the fix implemented in commit 9af152dcf1a0
>>> ("drm/gma500: Add NULL check for pci_gfx_root in mid_get_vbt_data()").
>>>
>>> Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
>>> ---
>>>    drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
>>> index e5a6f59af0b6..10b8f1fea1a2 100644
>>> --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
>>> +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
>>> @@ -2515,6 +2515,11 @@ static int pch_gbe_probe(struct pci_dev *pdev,
>>>                pci_get_domain_bus_and_slot(pci_domain_nr(adapter->pdev->bus),
>>>                                            adapter->pdev->bus->number,
>>>                                            PCI_DEVFN(12, 4));
>>> +     if (!adapter->ptp_pdev) {
>>> +             dev_err(&pdev->dev, "PTP device not found\n");
>>> +             ret = -ENODEV;
>>> +             goto err_free_netdev;
>>> +     }
>>
>> Why is this error fatal? I believe the device still can transmit and
>> receive packets without PTP device. If this situation is really possible
>> I would suggest you to add checks to ioctl function to remove
>> timestamping support if there is no PTP device found
> 
> Thanks for the prompt reply!
> Our static analysis tool found this issue and we made the initial
> patch based on the existings checks for pci_get_domain_bus_and_slot()
> 
> I've drafted a new version based on your suggestion. It removes the
> check from the probe function and instead adds the necessary NULL
> checks directly to the timestamping and ioctl functions.
> 
> Does the implementation below look correct to you? If so, I will
> prepare and send a formal v2 patch.
> 

I would say this patch looks way too defensive. It's enough to deny
enabling HW timestamping when there is no PTP device, then all other
checks will never happen. And I would keep the error message just to
inform users that PTP feature is not available on the particular
device/system.

Do you have HW to test your patch?