pch_gbe: Add NULL check for ptp_pdev in pch_gbe_probe()

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

Posted by Chenyuan Yang 2 months, 2 weeks ago

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;
+	}
 
 	netdev->netdev_ops = &pch_gbe_netdev_ops;
 	netdev->watchdog_timeo = PCH_GBE_WATCHDOG_PERIOD;
-- 
2.34.1

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

Posted by Vadim Fedorenko 2 months, 2 weeks ago

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

>   
>   	netdev->netdev_ops = &pch_gbe_netdev_ops;
>   	netdev->watchdog_timeo = PCH_GBE_WATCHDOG_PERIOD;

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

Posted by Chenyuan Yang 2 months, 2 weeks ago

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.

---
 drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 6 ++++++
 1 file changed, 6 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..ccef1b81e13b 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
@@ -136,6 +136,8 @@ pch_rx_timestamp(struct pch_gbe_adapter *adapter,
struct sk_buff *skb)

  /* Get ieee1588's dev information */
  pdev = adapter->ptp_pdev;
+ if (!pdev)
+ return;

  val = pch_ch_event_read(pdev);

@@ -174,6 +176,8 @@ pch_tx_timestamp(struct pch_gbe_adapter *adapter,
struct sk_buff *skb)

  /* Get ieee1588's dev information */
  pdev = adapter->ptp_pdev;
+ if (!pdev)
+ return;

  /*
  * This really stinks, but we have to poll for the Tx time stamp.
@@ -210,6 +214,8 @@ static int hwtstamp_ioctl(struct net_device
*netdev, struct ifreq *ifr, int cmd)

  /* Get ieee1588's dev information */
  pdev = adapter->ptp_pdev;
+ if (!pdev)
+ return -ENODEV;

  if (cfg.tx_type != HWTSTAMP_TX_OFF && cfg.tx_type != HWTSTAMP_TX_ON)
  return -ERANGE;
-- 


> >
> >       netdev->netdev_ops = &pch_gbe_netdev_ops;
> >       netdev->watchdog_timeo = PCH_GBE_WATCHDOG_PERIOD;
>

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

Posted by Vadim Fedorenko 2 months, 1 week ago

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?