[PATCH v2] net: bnxt_ptp: fix compilation error

[PATCH v2] net: bnxt_ptp: fix compilation error

[Damien Le Moal]

The Broadcom bnxt_ptp driver does not compile with GCC 11.2.2 when
CONFIG_WERROR is enabled. The following error is generated:

drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c: In function ‘bnxt_ptp_enable’:
drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:400:43: error: array
subscript 255 is above array bounds of ‘struct pps_pin[4]’
[-Werror=array-bounds]
  400 |  ptp->pps_info.pins[pin_id].event = BNXT_PPS_EVENT_EXTERNAL;
      |  ~~~~~~~~~~~~~~~~~~^~~~~~~~
In file included from drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:20:
drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h:75:24: note: while
referencing ‘pins’
   75 |         struct pps_pin pins[BNXT_MAX_TSIO_PINS];
      |                        ^~~~
cc1: all warnings being treated as errors

This is due to the function ptp_find_pin() returning a pin ID of -1 when
a valid pin is not found and this error never being checked.
Use the TSIO_PIN_VALID() macroin bnxt_ptp_enable() to check the result
of the calls to ptp_find_pin() in bnxt_ptp_enable() to fix this
compilation error.

Fixes: 9e518f25802c ("bnxt_en: 1PPS functions to configure TSIO pins")
Cc: <stable@vger.kernel.org>
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
Changes from v1:
* No need to change the TSIO_PIN_VALID() macro as pin_id is an unsigned
  value.

 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
index a0b321a19361..3c8fccbb9013 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
@@ -390,7 +390,7 @@ static int bnxt_ptp_enable(struct ptp_clock_info *ptp_info,
 		/* Configure an External PPS IN */
 		pin_id = ptp_find_pin(ptp->ptp_clock, PTP_PF_EXTTS,
 				      rq->extts.index);
-		if (!on)
+		if (!on || !TSIO_PIN_VALID(pin_id))
 			break;
 		rc = bnxt_ptp_cfg_pin(bp, pin_id, BNXT_PPS_PIN_PPS_IN);
 		if (rc)
@@ -403,7 +403,7 @@ static int bnxt_ptp_enable(struct ptp_clock_info *ptp_info,
 		/* Configure a Periodic PPS OUT */
 		pin_id = ptp_find_pin(ptp->ptp_clock, PTP_PF_PEROUT,
 				      rq->perout.index);
-		if (!on)
+		if (!on || !TSIO_PIN_VALID(pin_id))
 			break;
 
 		rc = bnxt_ptp_cfg_pin(bp, pin_id, BNXT_PPS_PIN_PPS_OUT);
-- 
2.35.1

Re: [PATCH v2] net: bnxt_ptp: fix compilation error

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 2997 bytes --]

On Sun, Mar 27, 2022 at 8:35 PM Damien Le Moal
<damien.lemoal@opensource.wdc.com> wrote:
>
> The Broadcom bnxt_ptp driver does not compile with GCC 11.2.2 when
> CONFIG_WERROR is enabled. The following error is generated:
>
> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c: In function ‘bnxt_ptp_enable’:
> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:400:43: error: array
> subscript 255 is above array bounds of ‘struct pps_pin[4]’
> [-Werror=array-bounds]
>   400 |  ptp->pps_info.pins[pin_id].event = BNXT_PPS_EVENT_EXTERNAL;
>       |  ~~~~~~~~~~~~~~~~~~^~~~~~~~
> In file included from drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:20:
> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h:75:24: note: while
> referencing ‘pins’
>    75 |         struct pps_pin pins[BNXT_MAX_TSIO_PINS];
>       |                        ^~~~
> cc1: all warnings being treated as errors
>
> This is due to the function ptp_find_pin() returning a pin ID of -1 when
> a valid pin is not found and this error never being checked.
> Use the TSIO_PIN_VALID() macroin bnxt_ptp_enable() to check the result
> of the calls to ptp_find_pin() in bnxt_ptp_enable() to fix this
> compilation error.
>
> Fixes: 9e518f25802c ("bnxt_en: 1PPS functions to configure TSIO pins")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
> Changes from v1:
> * No need to change the TSIO_PIN_VALID() macro as pin_id is an unsigned
>   value.
>
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> index a0b321a19361..3c8fccbb9013 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> @@ -390,7 +390,7 @@ static int bnxt_ptp_enable(struct ptp_clock_info *ptp_info,
>                 /* Configure an External PPS IN */
>                 pin_id = ptp_find_pin(ptp->ptp_clock, PTP_PF_EXTTS,
>                                       rq->extts.index);
> -               if (!on)
> +               if (!on || !TSIO_PIN_VALID(pin_id))

I think we need to return an error if !TSIO_PIN_VALID().  If we just
break, we'll still use pin_id after the switch statement.

>                         break;
>                 rc = bnxt_ptp_cfg_pin(bp, pin_id, BNXT_PPS_PIN_PPS_IN);
>                 if (rc)
> @@ -403,7 +403,7 @@ static int bnxt_ptp_enable(struct ptp_clock_info *ptp_info,
>                 /* Configure a Periodic PPS OUT */
>                 pin_id = ptp_find_pin(ptp->ptp_clock, PTP_PF_PEROUT,
>                                       rq->perout.index);
> -               if (!on)
> +               if (!on || !TSIO_PIN_VALID(pin_id))

Same here.

>                         break;
>
>                 rc = bnxt_ptp_cfg_pin(bp, pin_id, BNXT_PPS_PIN_PPS_OUT);
> --
> 2.35.1
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH v2] net: bnxt_ptp: fix compilation error

[Damien Le Moal]

On 3/28/22 14:36, Michael Chan wrote:
> On Sun, Mar 27, 2022 at 8:35 PM Damien Le Moal
> <damien.lemoal@opensource.wdc.com> wrote:
>>
>> The Broadcom bnxt_ptp driver does not compile with GCC 11.2.2 when
>> CONFIG_WERROR is enabled. The following error is generated:
>>
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c: In function ‘bnxt_ptp_enable’:
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:400:43: error: array
>> subscript 255 is above array bounds of ‘struct pps_pin[4]’
>> [-Werror=array-bounds]
>>   400 |  ptp->pps_info.pins[pin_id].event = BNXT_PPS_EVENT_EXTERNAL;
>>       |  ~~~~~~~~~~~~~~~~~~^~~~~~~~
>> In file included from drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:20:
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h:75:24: note: while
>> referencing ‘pins’
>>    75 |         struct pps_pin pins[BNXT_MAX_TSIO_PINS];
>>       |                        ^~~~
>> cc1: all warnings being treated as errors
>>
>> This is due to the function ptp_find_pin() returning a pin ID of -1 when
>> a valid pin is not found and this error never being checked.
>> Use the TSIO_PIN_VALID() macroin bnxt_ptp_enable() to check the result
>> of the calls to ptp_find_pin() in bnxt_ptp_enable() to fix this
>> compilation error.
>>
>> Fixes: 9e518f25802c ("bnxt_en: 1PPS functions to configure TSIO pins")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> ---
>> Changes from v1:
>> * No need to change the TSIO_PIN_VALID() macro as pin_id is an unsigned
>>   value.
>>
>>  drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> index a0b321a19361..3c8fccbb9013 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> @@ -390,7 +390,7 @@ static int bnxt_ptp_enable(struct ptp_clock_info *ptp_info,
>>                 /* Configure an External PPS IN */
>>                 pin_id = ptp_find_pin(ptp->ptp_clock, PTP_PF_EXTTS,
>>                                       rq->extts.index);
>> -               if (!on)
>> +               if (!on || !TSIO_PIN_VALID(pin_id))
> 
> I think we need to return an error if !TSIO_PIN_VALID().  If we just
> break, we'll still use pin_id after the switch statement.
> 
>>                         break;
>>                 rc = bnxt_ptp_cfg_pin(bp, pin_id, BNXT_PPS_PIN_PPS_IN);
>>                 if (rc)
>> @@ -403,7 +403,7 @@ static int bnxt_ptp_enable(struct ptp_clock_info *ptp_info,
>>                 /* Configure a Periodic PPS OUT */
>>                 pin_id = ptp_find_pin(ptp->ptp_clock, PTP_PF_PEROUT,
>>                                       rq->perout.index);
>> -               if (!on)
>> +               if (!on || !TSIO_PIN_VALID(pin_id))
> 
> Same here.

The call to bnxt_ptp_cfg_pin() after the swith will return -ENOTSUPP for
invalid pin IDs. So I did not feel like adding more changes was necessary.

We can return an error if you insist, but what should it be ? -EINVAL ?
-ENODEV ? -ENOTSUPP ? Given that bnxt_ptp_cfg_pin() return -ENOTSUPP, we
could use that code.

> 
>>                         break;
>>
>>                 rc = bnxt_ptp_cfg_pin(bp, pin_id, BNXT_PPS_PIN_PPS_OUT);
>> --
>> 2.35.1
>>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] net: bnxt_ptp: fix compilation error

[Pavan Chebbi]

[-- Attachment #1: Type: text/plain, Size: 4000 bytes --]

On Mon, Mar 28, 2022 at 11:14 AM Damien Le Moal
<damien.lemoal@opensource.wdc.com> wrote:
>
> On 3/28/22 14:36, Michael Chan wrote:
> > On Sun, Mar 27, 2022 at 8:35 PM Damien Le Moal
> > <damien.lemoal@opensource.wdc.com> wrote:
> >>
> >> The Broadcom bnxt_ptp driver does not compile with GCC 11.2.2 when
> >> CONFIG_WERROR is enabled. The following error is generated:
> >>
> >> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c: In function ‘bnxt_ptp_enable’:
> >> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:400:43: error: array
> >> subscript 255 is above array bounds of ‘struct pps_pin[4]’
> >> [-Werror=array-bounds]
> >>   400 |  ptp->pps_info.pins[pin_id].event = BNXT_PPS_EVENT_EXTERNAL;
> >>       |  ~~~~~~~~~~~~~~~~~~^~~~~~~~
> >> In file included from drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:20:
> >> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h:75:24: note: while
> >> referencing ‘pins’
> >>    75 |         struct pps_pin pins[BNXT_MAX_TSIO_PINS];
> >>       |                        ^~~~
> >> cc1: all warnings being treated as errors
> >>
> >> This is due to the function ptp_find_pin() returning a pin ID of -1 when
> >> a valid pin is not found and this error never being checked.
> >> Use the TSIO_PIN_VALID() macroin bnxt_ptp_enable() to check the result
> >> of the calls to ptp_find_pin() in bnxt_ptp_enable() to fix this
> >> compilation error.
> >>
> >> Fixes: 9e518f25802c ("bnxt_en: 1PPS functions to configure TSIO pins")
> >> Cc: <stable@vger.kernel.org>
> >> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> >> ---
> >> Changes from v1:
> >> * No need to change the TSIO_PIN_VALID() macro as pin_id is an unsigned
> >>   value.
> >>
> >>  drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> >> index a0b321a19361..3c8fccbb9013 100644
> >> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> >> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> >> @@ -390,7 +390,7 @@ static int bnxt_ptp_enable(struct ptp_clock_info *ptp_info,
> >>                 /* Configure an External PPS IN */
> >>                 pin_id = ptp_find_pin(ptp->ptp_clock, PTP_PF_EXTTS,
> >>                                       rq->extts.index);
> >> -               if (!on)
> >> +               if (!on || !TSIO_PIN_VALID(pin_id))
> >
> > I think we need to return an error if !TSIO_PIN_VALID().  If we just
> > break, we'll still use pin_id after the switch statement.
> >
> >>                         break;
> >>                 rc = bnxt_ptp_cfg_pin(bp, pin_id, BNXT_PPS_PIN_PPS_IN);
> >>                 if (rc)
> >> @@ -403,7 +403,7 @@ static int bnxt_ptp_enable(struct ptp_clock_info *ptp_info,
> >>                 /* Configure a Periodic PPS OUT */
> >>                 pin_id = ptp_find_pin(ptp->ptp_clock, PTP_PF_PEROUT,
> >>                                       rq->perout.index);
> >> -               if (!on)
> >> +               if (!on || !TSIO_PIN_VALID(pin_id))
> >
> > Same here.
>
> The call to bnxt_ptp_cfg_pin() after the swith will return -ENOTSUPP for
> invalid pin IDs. So I did not feel like adding more changes was necessary.
>
> We can return an error if you insist, but what should it be ? -EINVAL ?
> -ENODEV ? -ENOTSUPP ? Given that bnxt_ptp_cfg_pin() return -ENOTSUPP, we
> could use that code.

Would it not be better if we add a check only to validate the
ptp_find_pin is not returning -1
explicitly? TSIO_PIN_VALID validates just the MAX side. So I think
adding a check for -1 only
is valid and won't duplicate the code inside the two functions.

>
> >
> >>                         break;
> >>
> >>                 rc = bnxt_ptp_cfg_pin(bp, pin_id, BNXT_PPS_PIN_PPS_OUT);
> >> --
> >> 2.35.1
> >>
>
>
> --
> Damien Le Moal
> Western Digital Research

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH v2] net: bnxt_ptp: fix compilation error

[Damien Le Moal]

On 3/28/22 15:10, Pavan Chebbi wrote:
> On Mon, Mar 28, 2022 at 11:14 AM Damien Le Moal
> <damien.lemoal@opensource.wdc.com> wrote:
>>
>> On 3/28/22 14:36, Michael Chan wrote:
>>> On Sun, Mar 27, 2022 at 8:35 PM Damien Le Moal
>>> <damien.lemoal@opensource.wdc.com> wrote:
>>>>
>>>> The Broadcom bnxt_ptp driver does not compile with GCC 11.2.2 when
>>>> CONFIG_WERROR is enabled. The following error is generated:
>>>>
>>>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c: In function ‘bnxt_ptp_enable’:
>>>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:400:43: error: array
>>>> subscript 255 is above array bounds of ‘struct pps_pin[4]’
>>>> [-Werror=array-bounds]
>>>>   400 |  ptp->pps_info.pins[pin_id].event = BNXT_PPS_EVENT_EXTERNAL;
>>>>       |  ~~~~~~~~~~~~~~~~~~^~~~~~~~
>>>> In file included from drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:20:
>>>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h:75:24: note: while
>>>> referencing ‘pins’
>>>>    75 |         struct pps_pin pins[BNXT_MAX_TSIO_PINS];
>>>>       |                        ^~~~
>>>> cc1: all warnings being treated as errors
>>>>
>>>> This is due to the function ptp_find_pin() returning a pin ID of -1 when
>>>> a valid pin is not found and this error never being checked.
>>>> Use the TSIO_PIN_VALID() macroin bnxt_ptp_enable() to check the result
>>>> of the calls to ptp_find_pin() in bnxt_ptp_enable() to fix this
>>>> compilation error.
>>>>
>>>> Fixes: 9e518f25802c ("bnxt_en: 1PPS functions to configure TSIO pins")
>>>> Cc: <stable@vger.kernel.org>
>>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>>> ---
>>>> Changes from v1:
>>>> * No need to change the TSIO_PIN_VALID() macro as pin_id is an unsigned
>>>>   value.
>>>>
>>>>  drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>>>> index a0b321a19361..3c8fccbb9013 100644
>>>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>>>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>>>> @@ -390,7 +390,7 @@ static int bnxt_ptp_enable(struct ptp_clock_info *ptp_info,
>>>>                 /* Configure an External PPS IN */
>>>>                 pin_id = ptp_find_pin(ptp->ptp_clock, PTP_PF_EXTTS,
>>>>                                       rq->extts.index);
>>>> -               if (!on)
>>>> +               if (!on || !TSIO_PIN_VALID(pin_id))
>>>
>>> I think we need to return an error if !TSIO_PIN_VALID().  If we just
>>> break, we'll still use pin_id after the switch statement.
>>>
>>>>                         break;
>>>>                 rc = bnxt_ptp_cfg_pin(bp, pin_id, BNXT_PPS_PIN_PPS_IN);
>>>>                 if (rc)
>>>> @@ -403,7 +403,7 @@ static int bnxt_ptp_enable(struct ptp_clock_info *ptp_info,
>>>>                 /* Configure a Periodic PPS OUT */
>>>>                 pin_id = ptp_find_pin(ptp->ptp_clock, PTP_PF_PEROUT,
>>>>                                       rq->perout.index);
>>>> -               if (!on)
>>>> +               if (!on || !TSIO_PIN_VALID(pin_id))
>>>
>>> Same here.
>>
>> The call to bnxt_ptp_cfg_pin() after the swith will return -ENOTSUPP for
>> invalid pin IDs. So I did not feel like adding more changes was necessary.
>>
>> We can return an error if you insist, but what should it be ? -EINVAL ?
>> -ENODEV ? -ENOTSUPP ? Given that bnxt_ptp_cfg_pin() return -ENOTSUPP, we
>> could use that code.
> 
> Would it not be better if we add a check only to validate the
> ptp_find_pin is not returning -1
> explicitly? TSIO_PIN_VALID validates just the MAX side. So I think
> adding a check for -1 only
> is valid and won't duplicate the code inside the two functions.

I did that in v1, but pin_id is unsigned (u8). So changing
TSIO_PIN_VALID() to check for "pin >= 0" is a bit silly in this case.
But looking at other drivers using ptp_find_pin(), many do check the
return value first...

Sending a v3 with that check added.

> 
>>
>>>
>>>>                         break;
>>>>
>>>>                 rc = bnxt_ptp_cfg_pin(bp, pin_id, BNXT_PPS_PIN_PPS_OUT);
>>>> --
>>>> 2.35.1
>>>>
>>
>>
>> --
>> Damien Le Moal
>> Western Digital Research


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] net: bnxt_ptp: fix compilation error

[Andrew Lunn]

> The call to bnxt_ptp_cfg_pin() after the swith will return -ENOTSUPP for
> invalid pin IDs. So I did not feel like adding more changes was necessary.
> 
> We can return an error if you insist, but what should it be ? -EINVAL ?
> -ENODEV ? -ENOTSUPP ? Given that bnxt_ptp_cfg_pin() return -ENOTSUPP, we
> could use that code.

https://elixir.bootlin.com/linux/v5.17.1/source/include/linux/errno.h#L23

ENOTSUPP is an NFS only error code. It should not be used anywhere
else. EOPNOTSUPP is the generic error that should be used. However,
don't replace an ENOTSUPP with an EOPNOTSUPP without considering ABI.

    Andrew

Re: [PATCH v2] net: bnxt_ptp: fix compilation error

[Damien Le Moal]

On 3/28/22 21:46, Andrew Lunn wrote:
>> The call to bnxt_ptp_cfg_pin() after the swith will return -ENOTSUPP for
>> invalid pin IDs. So I did not feel like adding more changes was necessary.
>>
>> We can return an error if you insist, but what should it be ? -EINVAL ?
>> -ENODEV ? -ENOTSUPP ? Given that bnxt_ptp_cfg_pin() return -ENOTSUPP, we
>> could use that code.
> 
> https://elixir.bootlin.com/linux/v5.17.1/source/include/linux/errno.h#L23
> 
> ENOTSUPP is an NFS only error code. It should not be used anywhere
> else. EOPNOTSUPP is the generic error that should be used. However,
> don't replace an ENOTSUPP with an EOPNOTSUPP without considering ABI.

Typo... the current error for invalid pin IDs is EOPNOTSUPP, not ENOTSUPP.
So I reused EOPNOTSUPP in the patch.

> 
>     Andrew


-- 
Damien Le Moal
Western Digital Research