| Message ID | 20220328033540.189778-1-damien.lemoal@opensource.wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: bnxt_ptp: fix compilation error | expand |
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 >
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 >>
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
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
> 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
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
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);
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(-)