| Message ID | 20220328062708.207079-1-damien.lemoal@opensource.wdc.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | dcf500065fabe27676dfe7b4ba521a4f1e0fc8ac |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3] net: bnxt_ptp: fix compilation error | expand |
On Mon, Mar 28, 2022 at 11:57 AM 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. > Change the TSIO_PIN_VALID() function to also check that a pin ID is not > negative and use this macro in bnxt_ptp_enable() to check the result of > the calls to ptp_find_pin() to return an error early for invalid pins. > This fixes the 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 v2: > * Restore the improved check in TSIO_PIN_VALID() and use this macro to > return an error early in bnxt_ptp_enable() in case of invalid pin ID. > 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 | 6 +++++- > drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 2 +- > 2 files changed, 6 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..9c2ad5e67a5d 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c > @@ -382,7 +382,7 @@ static int bnxt_ptp_enable(struct ptp_clock_info *ptp_info, > struct bnxt_ptp_cfg *ptp = container_of(ptp_info, struct bnxt_ptp_cfg, > ptp_info); > struct bnxt *bp = ptp->bp; > - u8 pin_id; > + int pin_id; > int rc; > > switch (rq->type) { > @@ -390,6 +390,8 @@ 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 (!TSIO_PIN_VALID(pin_id)) > + return -EOPNOTSUPP; Thanks. Could we now remove this check from the function bnxt_ptp_cfg_pin() ? > if (!on) > break; > rc = bnxt_ptp_cfg_pin(bp, pin_id, BNXT_PPS_PIN_PPS_IN); > @@ -403,6 +405,8 @@ 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 (!TSIO_PIN_VALID(pin_id)) > + return -EOPNOTSUPP; > if (!on) > break; > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h > index 373baf45884b..530b9922608c 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h > @@ -31,7 +31,7 @@ struct pps_pin { > u8 state; > }; > > -#define TSIO_PIN_VALID(pin) ((pin) < (BNXT_MAX_TSIO_PINS)) > +#define TSIO_PIN_VALID(pin) ((pin) >= 0 && (pin) < (BNXT_MAX_TSIO_PINS)) > > #define EVENT_DATA2_PPS_EVENT_TYPE(data2) \ > ((data2) & ASYNC_EVENT_CMPL_PPS_TIMESTAMP_EVENT_DATA2_EVENT_TYPE) > -- > 2.35.1 >
On 3/28/22 15:38, Pavan Chebbi wrote: > On Mon, Mar 28, 2022 at 11:57 AM 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. >> Change the TSIO_PIN_VALID() function to also check that a pin ID is not >> negative and use this macro in bnxt_ptp_enable() to check the result of >> the calls to ptp_find_pin() to return an error early for invalid pins. >> This fixes the 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 v2: >> * Restore the improved check in TSIO_PIN_VALID() and use this macro to >> return an error early in bnxt_ptp_enable() in case of invalid pin ID. >> 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 | 6 +++++- >> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 2 +- >> 2 files changed, 6 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..9c2ad5e67a5d 100644 >> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c >> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c >> @@ -382,7 +382,7 @@ static int bnxt_ptp_enable(struct ptp_clock_info *ptp_info, >> struct bnxt_ptp_cfg *ptp = container_of(ptp_info, struct bnxt_ptp_cfg, >> ptp_info); >> struct bnxt *bp = ptp->bp; >> - u8 pin_id; >> + int pin_id; >> int rc; >> >> switch (rq->type) { >> @@ -390,6 +390,8 @@ 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 (!TSIO_PIN_VALID(pin_id)) >> + return -EOPNOTSUPP; > > Thanks. Could we now remove this check from the function bnxt_ptp_cfg_pin() ? Having a quick glance at all the call sites, it looks like it would be OK. But may be in a different patch ? This patch is not actually fixing any real problem. It is only fixing gcc not being able to detect that pin_id can never be with an invalid value since bnxt_ptp_cfg_pin() would fail before pin_id is used in the assignment in bnxt_ptp_enable(). Thoughts ? > >> if (!on) >> break; >> rc = bnxt_ptp_cfg_pin(bp, pin_id, BNXT_PPS_PIN_PPS_IN); >> @@ -403,6 +405,8 @@ 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 (!TSIO_PIN_VALID(pin_id)) >> + return -EOPNOTSUPP; >> if (!on) >> break; >> >> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h >> index 373baf45884b..530b9922608c 100644 >> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h >> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h >> @@ -31,7 +31,7 @@ struct pps_pin { >> u8 state; >> }; >> >> -#define TSIO_PIN_VALID(pin) ((pin) < (BNXT_MAX_TSIO_PINS)) >> +#define TSIO_PIN_VALID(pin) ((pin) >= 0 && (pin) < (BNXT_MAX_TSIO_PINS)) >> >> #define EVENT_DATA2_PPS_EVENT_TYPE(data2) \ >> ((data2) & ASYNC_EVENT_CMPL_PPS_TIMESTAMP_EVENT_DATA2_EVENT_TYPE) >> -- >> 2.35.1 >>
On Mon, Mar 28, 2022 at 12:01 AM Damien Le Moal <damien.lemoal@opensource.wdc.com> wrote: > > On 3/28/22 15:38, Pavan Chebbi wrote: > > On Mon, Mar 28, 2022 at 11:57 AM 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. > >> Change the TSIO_PIN_VALID() function to also check that a pin ID is not > >> negative and use this macro in bnxt_ptp_enable() to check the result of > >> the calls to ptp_find_pin() to return an error early for invalid pins. > >> This fixes the 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 v2: > >> * Restore the improved check in TSIO_PIN_VALID() and use this macro to > >> return an error early in bnxt_ptp_enable() in case of invalid pin ID. > >> 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 | 6 +++++- > >> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 2 +- > >> 2 files changed, 6 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..9c2ad5e67a5d 100644 > >> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c > >> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c > >> @@ -382,7 +382,7 @@ static int bnxt_ptp_enable(struct ptp_clock_info *ptp_info, > >> struct bnxt_ptp_cfg *ptp = container_of(ptp_info, struct bnxt_ptp_cfg, > >> ptp_info); > >> struct bnxt *bp = ptp->bp; > >> - u8 pin_id; > >> + int pin_id; > >> int rc; > >> > >> switch (rq->type) { > >> @@ -390,6 +390,8 @@ 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 (!TSIO_PIN_VALID(pin_id)) > >> + return -EOPNOTSUPP; > > > > Thanks. Could we now remove this check from the function bnxt_ptp_cfg_pin() ? > > Having a quick glance at all the call sites, it looks like it would be OK. > But may be in a different patch ? > Yes, we can improve it later in a separate patch. Thanks. Reviewed-by: Michael Chan <michael.chan@broadcom.com>
Hello: This patch was applied to netdev/net.git (master) by Jakub Kicinski <kuba@kernel.org>: On Mon, 28 Mar 2022 15:27:08 +0900 you 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 > > [...] Here is the summary with links: - [v3] net: bnxt_ptp: fix compilation error https://git.kernel.org/netdev/net/c/dcf500065fab You are awesome, thank you!
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c index a0b321a19361..9c2ad5e67a5d 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c @@ -382,7 +382,7 @@ static int bnxt_ptp_enable(struct ptp_clock_info *ptp_info, struct bnxt_ptp_cfg *ptp = container_of(ptp_info, struct bnxt_ptp_cfg, ptp_info); struct bnxt *bp = ptp->bp; - u8 pin_id; + int pin_id; int rc; switch (rq->type) { @@ -390,6 +390,8 @@ 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 (!TSIO_PIN_VALID(pin_id)) + return -EOPNOTSUPP; if (!on) break; rc = bnxt_ptp_cfg_pin(bp, pin_id, BNXT_PPS_PIN_PPS_IN); @@ -403,6 +405,8 @@ 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 (!TSIO_PIN_VALID(pin_id)) + return -EOPNOTSUPP; if (!on) break; diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h index 373baf45884b..530b9922608c 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h @@ -31,7 +31,7 @@ struct pps_pin { u8 state; }; -#define TSIO_PIN_VALID(pin) ((pin) < (BNXT_MAX_TSIO_PINS)) +#define TSIO_PIN_VALID(pin) ((pin) >= 0 && (pin) < (BNXT_MAX_TSIO_PINS)) #define EVENT_DATA2_PPS_EVENT_TYPE(data2) \ ((data2) & ASYNC_EVENT_CMPL_PPS_TIMESTAMP_EVENT_DATA2_EVENT_TYPE)
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. Change the TSIO_PIN_VALID() function to also check that a pin ID is not negative and use this macro in bnxt_ptp_enable() to check the result of the calls to ptp_find_pin() to return an error early for invalid pins. This fixes the 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 v2: * Restore the improved check in TSIO_PIN_VALID() and use this macro to return an error early in bnxt_ptp_enable() in case of invalid pin ID. 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 | 6 +++++- drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-)