| Message ID | 20210127085752.120571-8-aik@ozlabs.ru |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Compile with -Wextra | expand |
On 27/01/2021 09.57, Alexey Kardashevskiy wrote: > -Wextra enables a bunch of rather useful checks which this fixes. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > lib/libe1k/e1k.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/lib/libe1k/e1k.c b/lib/libe1k/e1k.c > index 4dd7d2eb97c2..3bbc75fba199 100644 > --- a/lib/libe1k/e1k.c > +++ b/lib/libe1k/e1k.c > @@ -163,7 +163,7 @@ static const e1k_dev_t e1k_dev[] = { > { 0x1016, E1K_82540, "82540EP Mobile" }, > { 0x1017, E1K_82540, "82540EP Desktop" }, > { 0x100E, E1K_82540, "82540EM Desktop" }, > - { 0 , 0 } > + { 0 } > }; Ack for the above change (or you could simply add a NULL as third parameter), but are the changes below really required? Seem like you only tried to get rid of the unused-parameter warnings which you finally switched off anyway? Thomas > /* > @@ -182,8 +182,8 @@ check_driver(uint16_t vendor_id, uint16_t device_id); > > static int e1k_init(net_driver_t *driver); > static int e1k_term(void); > -static int e1k_xmit(char *f_buffer_pc, int f_len_i); > -static int e1k_receive(char *f_buffer_pc, int f_len_i); > +static int e1k_xmit(char *f_buffer_pc, unsigned f_len_i); > +static int e1k_receive(char *f_buffer_pc, unsigned f_len_i); > > /** > * Translate virtual to "physical" address, ie. an address > @@ -549,11 +549,11 @@ e1k_mac_init(uint8_t *f_mac_pu08) > * e1k_receive > */ > static int > -e1k_receive(char *f_buffer_pc, int f_len_i) > +e1k_receive(char *f_buffer_pc, unsigned f_len_i) > { > uint32_t l_rdh_u32 = e1k_rd32(RDH); // this includes needed dummy read > e1k_rx_desc_st *rx; > - int l_ret_i; > + unsigned l_ret_i; > > #ifdef E1K_DEBUG > #ifdef E1K_SHOW_RCV_DATA > @@ -589,11 +589,11 @@ e1k_receive(char *f_buffer_pc, int f_len_i) > * copy the data > */ > memcpy((uint8_t *) f_buffer_pc, dma2virt(bswap_64(rx->m_buffer_u64)), > - (size_t) l_ret_i); > + (size_t) MIN(l_ret_i, f_len_i)); > > #ifdef E1K_DEBUG > #if defined(E1K_SHOW_RCV) || defined(E1K_SHOW_RCV_DATA) > - printf("e1k: %d bytes received\n", l_ret_i); > + printf("e1k: %d bytes received (max %d)\n", l_ret_i, f_len_i); > #endif > > #ifdef E1K_SHOW_RCV_DATA > @@ -631,7 +631,7 @@ e1k_receive(char *f_buffer_pc, int f_len_i) > } > > static int > -e1k_xmit(char *f_buffer_pc, int f_len_i) > +e1k_xmit(char *f_buffer_pc, unsigned f_len_i) > { > uint32_t l_tdh_u32 = e1k_rd32(TDH); > uint32_t l_tdt_u32 = e1k_rd32(TDT); >
On 29/01/2021 02:04, Thomas Huth wrote: > On 27/01/2021 09.57, Alexey Kardashevskiy wrote: >> -Wextra enables a bunch of rather useful checks which this fixes. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> lib/libe1k/e1k.c | 16 ++++++++-------- >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/lib/libe1k/e1k.c b/lib/libe1k/e1k.c >> index 4dd7d2eb97c2..3bbc75fba199 100644 >> --- a/lib/libe1k/e1k.c >> +++ b/lib/libe1k/e1k.c >> @@ -163,7 +163,7 @@ static const e1k_dev_t e1k_dev[] = { >> { 0x1016, E1K_82540, "82540EP Mobile" }, >> { 0x1017, E1K_82540, "82540EP Desktop" }, >> { 0x100E, E1K_82540, "82540EM Desktop" }, >> - { 0 , 0 } >> + { 0 } >> }; > > Ack for the above change (or you could simply add a NULL as third > parameter), but are the changes below really required? These was a warning that the third parameter is not initialized. So it could be either one zero or three zeroes. I find one more future proof. > Seem like you > only tried to get rid of the unused-parameter warnings which you finally > switched off anyway? No, it is a different warning. Thanks, > > Thomas > > >> /* >> @@ -182,8 +182,8 @@ check_driver(uint16_t vendor_id, uint16_t device_id); >> static int e1k_init(net_driver_t *driver); >> static int e1k_term(void); >> -static int e1k_xmit(char *f_buffer_pc, int f_len_i); >> -static int e1k_receive(char *f_buffer_pc, int f_len_i); >> +static int e1k_xmit(char *f_buffer_pc, unsigned f_len_i); >> +static int e1k_receive(char *f_buffer_pc, unsigned f_len_i); >> /** >> * Translate virtual to "physical" address, ie. an address >> @@ -549,11 +549,11 @@ e1k_mac_init(uint8_t *f_mac_pu08) >> * e1k_receive >> */ >> static int >> -e1k_receive(char *f_buffer_pc, int f_len_i) >> +e1k_receive(char *f_buffer_pc, unsigned f_len_i) >> { >> uint32_t l_rdh_u32 = e1k_rd32(RDH); // this includes >> needed dummy read >> e1k_rx_desc_st *rx; >> - int l_ret_i; >> + unsigned l_ret_i; >> #ifdef E1K_DEBUG >> #ifdef E1K_SHOW_RCV_DATA >> @@ -589,11 +589,11 @@ e1k_receive(char *f_buffer_pc, int f_len_i) >> * copy the data >> */ >> memcpy((uint8_t *) f_buffer_pc, >> dma2virt(bswap_64(rx->m_buffer_u64)), >> - (size_t) l_ret_i); >> + (size_t) MIN(l_ret_i, f_len_i)); >> #ifdef E1K_DEBUG >> #if defined(E1K_SHOW_RCV) || defined(E1K_SHOW_RCV_DATA) >> - printf("e1k: %d bytes received\n", l_ret_i); >> + printf("e1k: %d bytes received (max %d)\n", l_ret_i, f_len_i); >> #endif >> #ifdef E1K_SHOW_RCV_DATA >> @@ -631,7 +631,7 @@ e1k_receive(char *f_buffer_pc, int f_len_i) >> } >> static int >> -e1k_xmit(char *f_buffer_pc, int f_len_i) >> +e1k_xmit(char *f_buffer_pc, unsigned f_len_i) >> { >> uint32_t l_tdh_u32 = e1k_rd32(TDH); >> uint32_t l_tdt_u32 = e1k_rd32(TDT); >> >
On 29/01/2021 03.01, Alexey Kardashevskiy wrote: > > > On 29/01/2021 02:04, Thomas Huth wrote: >> On 27/01/2021 09.57, Alexey Kardashevskiy wrote: >>> -Wextra enables a bunch of rather useful checks which this fixes. >>> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> --- >>> lib/libe1k/e1k.c | 16 ++++++++-------- >>> 1 file changed, 8 insertions(+), 8 deletions(-) >>> >>> diff --git a/lib/libe1k/e1k.c b/lib/libe1k/e1k.c >>> index 4dd7d2eb97c2..3bbc75fba199 100644 >>> --- a/lib/libe1k/e1k.c >>> +++ b/lib/libe1k/e1k.c >>> @@ -163,7 +163,7 @@ static const e1k_dev_t e1k_dev[] = { >>> { 0x1016, E1K_82540, "82540EP Mobile" }, >>> { 0x1017, E1K_82540, "82540EP Desktop" }, >>> { 0x100E, E1K_82540, "82540EM Desktop" }, >>> - { 0 , 0 } >>> + { 0 } >>> }; >> >> Ack for the above change (or you could simply add a NULL as third >> parameter), but are the changes below really required? > > > These was a warning that the third parameter is not initialized. So it could > be either one zero or three zeroes. I find one more future proof. > > >> Seem like you only tried to get rid of the unused-parameter warnings which >> you finally switched off anyway? > > No, it is a different warning. Thanks, So which warnings do you get that you tried to address with the changes below? I only get one warning here with -Wextra -Wno-unused-parameter, and that's the one that you fixed with above hunk. Thomas >> >>> /* >>> @@ -182,8 +182,8 @@ check_driver(uint16_t vendor_id, uint16_t device_id); >>> static int e1k_init(net_driver_t *driver); >>> static int e1k_term(void); >>> -static int e1k_xmit(char *f_buffer_pc, int f_len_i); >>> -static int e1k_receive(char *f_buffer_pc, int f_len_i); >>> +static int e1k_xmit(char *f_buffer_pc, unsigned f_len_i); >>> +static int e1k_receive(char *f_buffer_pc, unsigned f_len_i); >>> /** >>> * Translate virtual to "physical" address, ie. an address >>> @@ -549,11 +549,11 @@ e1k_mac_init(uint8_t *f_mac_pu08) >>> * e1k_receive >>> */ >>> static int >>> -e1k_receive(char *f_buffer_pc, int f_len_i) >>> +e1k_receive(char *f_buffer_pc, unsigned f_len_i) >>> { >>> uint32_t l_rdh_u32 = e1k_rd32(RDH); // this includes needed >>> dummy read >>> e1k_rx_desc_st *rx; >>> - int l_ret_i; >>> + unsigned l_ret_i; >>> #ifdef E1K_DEBUG >>> #ifdef E1K_SHOW_RCV_DATA >>> @@ -589,11 +589,11 @@ e1k_receive(char *f_buffer_pc, int f_len_i) >>> * copy the data >>> */ >>> memcpy((uint8_t *) f_buffer_pc, dma2virt(bswap_64(rx->m_buffer_u64)), >>> - (size_t) l_ret_i); >>> + (size_t) MIN(l_ret_i, f_len_i)); >>> #ifdef E1K_DEBUG >>> #if defined(E1K_SHOW_RCV) || defined(E1K_SHOW_RCV_DATA) >>> - printf("e1k: %d bytes received\n", l_ret_i); >>> + printf("e1k: %d bytes received (max %d)\n", l_ret_i, f_len_i); >>> #endif >>> #ifdef E1K_SHOW_RCV_DATA >>> @@ -631,7 +631,7 @@ e1k_receive(char *f_buffer_pc, int f_len_i) >>> } >>> static int >>> -e1k_xmit(char *f_buffer_pc, int f_len_i) >>> +e1k_xmit(char *f_buffer_pc, unsigned f_len_i) >>> { >>> uint32_t l_tdh_u32 = e1k_rd32(TDH); >>> uint32_t l_tdt_u32 = e1k_rd32(TDT); >>> >> >
On 29/01/2021 17:38, Thomas Huth wrote: > On 29/01/2021 03.01, Alexey Kardashevskiy wrote: >> >> >> On 29/01/2021 02:04, Thomas Huth wrote: >>> On 27/01/2021 09.57, Alexey Kardashevskiy wrote: >>>> -Wextra enables a bunch of rather useful checks which this fixes. >>>> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>>> --- >>>> lib/libe1k/e1k.c | 16 ++++++++-------- >>>> 1 file changed, 8 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/lib/libe1k/e1k.c b/lib/libe1k/e1k.c >>>> index 4dd7d2eb97c2..3bbc75fba199 100644 >>>> --- a/lib/libe1k/e1k.c >>>> +++ b/lib/libe1k/e1k.c >>>> @@ -163,7 +163,7 @@ static const e1k_dev_t e1k_dev[] = { >>>> { 0x1016, E1K_82540, "82540EP Mobile" }, >>>> { 0x1017, E1K_82540, "82540EP Desktop" }, >>>> { 0x100E, E1K_82540, "82540EM Desktop" }, >>>> - { 0 , 0 } >>>> + { 0 } >>>> }; >>> >>> Ack for the above change (or you could simply add a NULL as third >>> parameter), but are the changes below really required? >> >> >> These was a warning that the third parameter is not initialized. So it >> could be either one zero or three zeroes. I find one more future proof. >> >> >>> Seem like you only tried to get rid of the unused-parameter warnings >>> which you finally switched off anyway? >> >> No, it is a different warning. Thanks, > > So which warnings do you get that you tried to address with the changes > below? I only get one warning here with -Wextra -Wno-unused-parameter, > and that's the one that you fixed with above hunk. The initial warning was about unused parameter. Which I decided to use, hence this new MIN() call down there which in turn triggered signed/unsigned comparison so I changed the type. Makes sense? > > Thomas > > >>> >>>> /* >>>> @@ -182,8 +182,8 @@ check_driver(uint16_t vendor_id, uint16_t >>>> device_id); >>>> static int e1k_init(net_driver_t *driver); >>>> static int e1k_term(void); >>>> -static int e1k_xmit(char *f_buffer_pc, int f_len_i); >>>> -static int e1k_receive(char *f_buffer_pc, int f_len_i); >>>> +static int e1k_xmit(char *f_buffer_pc, unsigned f_len_i); >>>> +static int e1k_receive(char *f_buffer_pc, unsigned f_len_i); >>>> /** >>>> * Translate virtual to "physical" address, ie. an address >>>> @@ -549,11 +549,11 @@ e1k_mac_init(uint8_t *f_mac_pu08) >>>> * e1k_receive >>>> */ >>>> static int >>>> -e1k_receive(char *f_buffer_pc, int f_len_i) >>>> +e1k_receive(char *f_buffer_pc, unsigned f_len_i) >>>> { >>>> uint32_t l_rdh_u32 = e1k_rd32(RDH); // this includes >>>> needed dummy read >>>> e1k_rx_desc_st *rx; >>>> - int l_ret_i; >>>> + unsigned l_ret_i; >>>> #ifdef E1K_DEBUG >>>> #ifdef E1K_SHOW_RCV_DATA >>>> @@ -589,11 +589,11 @@ e1k_receive(char *f_buffer_pc, int f_len_i) >>>> * copy the data >>>> */ >>>> memcpy((uint8_t *) f_buffer_pc, >>>> dma2virt(bswap_64(rx->m_buffer_u64)), >>>> - (size_t) l_ret_i); >>>> + (size_t) MIN(l_ret_i, f_len_i)); >>>> #ifdef E1K_DEBUG >>>> #if defined(E1K_SHOW_RCV) || defined(E1K_SHOW_RCV_DATA) >>>> - printf("e1k: %d bytes received\n", l_ret_i); >>>> + printf("e1k: %d bytes received (max %d)\n", l_ret_i, f_len_i); >>>> #endif >>>> #ifdef E1K_SHOW_RCV_DATA >>>> @@ -631,7 +631,7 @@ e1k_receive(char *f_buffer_pc, int f_len_i) >>>> } >>>> static int >>>> -e1k_xmit(char *f_buffer_pc, int f_len_i) >>>> +e1k_xmit(char *f_buffer_pc, unsigned f_len_i) >>>> { >>>> uint32_t l_tdh_u32 = e1k_rd32(TDH); >>>> uint32_t l_tdt_u32 = e1k_rd32(TDT); >>>> >>> >> >
diff --git a/lib/libe1k/e1k.c b/lib/libe1k/e1k.c index 4dd7d2eb97c2..3bbc75fba199 100644 --- a/lib/libe1k/e1k.c +++ b/lib/libe1k/e1k.c @@ -163,7 +163,7 @@ static const e1k_dev_t e1k_dev[] = { { 0x1016, E1K_82540, "82540EP Mobile" }, { 0x1017, E1K_82540, "82540EP Desktop" }, { 0x100E, E1K_82540, "82540EM Desktop" }, - { 0 , 0 } + { 0 } }; /* @@ -182,8 +182,8 @@ check_driver(uint16_t vendor_id, uint16_t device_id); static int e1k_init(net_driver_t *driver); static int e1k_term(void); -static int e1k_xmit(char *f_buffer_pc, int f_len_i); -static int e1k_receive(char *f_buffer_pc, int f_len_i); +static int e1k_xmit(char *f_buffer_pc, unsigned f_len_i); +static int e1k_receive(char *f_buffer_pc, unsigned f_len_i); /** * Translate virtual to "physical" address, ie. an address @@ -549,11 +549,11 @@ e1k_mac_init(uint8_t *f_mac_pu08) * e1k_receive */ static int -e1k_receive(char *f_buffer_pc, int f_len_i) +e1k_receive(char *f_buffer_pc, unsigned f_len_i) { uint32_t l_rdh_u32 = e1k_rd32(RDH); // this includes needed dummy read e1k_rx_desc_st *rx; - int l_ret_i; + unsigned l_ret_i; #ifdef E1K_DEBUG #ifdef E1K_SHOW_RCV_DATA @@ -589,11 +589,11 @@ e1k_receive(char *f_buffer_pc, int f_len_i) * copy the data */ memcpy((uint8_t *) f_buffer_pc, dma2virt(bswap_64(rx->m_buffer_u64)), - (size_t) l_ret_i); + (size_t) MIN(l_ret_i, f_len_i)); #ifdef E1K_DEBUG #if defined(E1K_SHOW_RCV) || defined(E1K_SHOW_RCV_DATA) - printf("e1k: %d bytes received\n", l_ret_i); + printf("e1k: %d bytes received (max %d)\n", l_ret_i, f_len_i); #endif #ifdef E1K_SHOW_RCV_DATA @@ -631,7 +631,7 @@ e1k_receive(char *f_buffer_pc, int f_len_i) } static int -e1k_xmit(char *f_buffer_pc, int f_len_i) +e1k_xmit(char *f_buffer_pc, unsigned f_len_i) { uint32_t l_tdh_u32 = e1k_rd32(TDH); uint32_t l_tdt_u32 = e1k_rd32(TDT);
-Wextra enables a bunch of rather useful checks which this fixes. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- lib/libe1k/e1k.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)