From: Arnd Bergmann <[email protected]>
Building a "W=1" kernel with clang produces a warning about
suspicous pointer arithmetic:
drivers/atm/horizon.c:1844:52: warning: performing pointer arithmetic
on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
for (mem = (HDW *) memmap; mem < (HDW *) (memmap + 1); ++mem)
The way that the addresses are handled is very obscure, and
rewriting it to be more conventional seems fairly pointless, given
that this driver probably has no users.
Shut up this warning by adding a cast to uintptr_t.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/atm/horizon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/atm/horizon.c b/drivers/atm/horizon.c
index 4f2951cbe69c..cd368786b216 100644
--- a/drivers/atm/horizon.c
+++ b/drivers/atm/horizon.c
@@ -1841,7 +1841,7 @@ static int hrz_init(hrz_dev *dev)
printk (" clearing memory");
- for (mem = (HDW *) memmap; mem < (HDW *) (memmap + 1); ++mem)
+ for (mem = (HDW *) memmap; mem < (HDW *) ((uintptr_t)memmap + 1); ++mem)
wr_mem (dev, mem, 0);
printk (" tx channels");
--
2.27.0
From: Arnd Bergmann <[email protected]>
The returned string from rsxx_card_state_to_str is 'const',
but the other qualifier doesn't change anything here except
causing a warning with 'clang -Wextra':
drivers/block/rsxx/core.c:393:21: warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers]
static const char * const rsxx_card_state_to_str(unsigned int state)
Fixes: f37912039eb0 ("block: IBM RamSan 70/80 trivial changes.")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/block/rsxx/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/block/rsxx/core.c b/drivers/block/rsxx/core.c
index 63f549889f87..d0af46d7b681 100644
--- a/drivers/block/rsxx/core.c
+++ b/drivers/block/rsxx/core.c
@@ -390,7 +390,7 @@ static irqreturn_t rsxx_isr(int irq, void *pdata)
}
/*----------------- Card Event Handler -------------------*/
-static const char * const rsxx_card_state_to_str(unsigned int state)
+static const char *rsxx_card_state_to_str(unsigned int state)
{
static const char * const state_strings[] = {
"Unknown", "Shutdown", "Starting", "Formatting",
--
2.27.0
From: Arnd Bergmann <[email protected]>
gcc -Wextra points out multiple fields that use the same index '1'
in the wimax_gnl_policy definition:
net/wimax/stack.c:393:29: warning: initialized field overwritten [-Woverride-init]
net/wimax/stack.c:397:28: warning: initialized field overwritten [-Woverride-init]
net/wimax/stack.c:398:26: warning: initialized field overwritten [-Woverride-init]
This seems to work since all four use the same NLA_U32 value, but it
still appears to be wrong. In addition, there is no intializer for
WIMAX_GNL_MSG_PIPE_NAME, which uses the same index '2' as
WIMAX_GNL_RFKILL_STATE.
Johannes already changed this twice to improve it, but I don't think
there is a good solution, so try to work around it by using a
numeric index and adding comments.
Cc: Johannes Berg <[email protected]>
Fixes: 3b0f31f2b8c9 ("genetlink: make policy common to family")
Fixes: b61a5eea5904 ("wimax: use genl_register_family_with_ops()")
Signed-off-by: Arnd Bergmann <[email protected]>
---
net/wimax/stack.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/net/wimax/stack.c b/net/wimax/stack.c
index b6dd9d956ed8..3a62af3f80bf 100644
--- a/net/wimax/stack.c
+++ b/net/wimax/stack.c
@@ -388,17 +388,24 @@ void wimax_dev_init(struct wimax_dev *wimax_dev)
}
EXPORT_SYMBOL_GPL(wimax_dev_init);
+/*
+ * There are multiple enums reusing the same values, adding
+ * others is only possible if they use a compatible policy.
+ */
static const struct nla_policy wimax_gnl_policy[WIMAX_GNL_ATTR_MAX + 1] = {
- [WIMAX_GNL_RESET_IFIDX] = { .type = NLA_U32, },
- [WIMAX_GNL_RFKILL_IFIDX] = { .type = NLA_U32, },
- [WIMAX_GNL_RFKILL_STATE] = {
- .type = NLA_U32 /* enum wimax_rf_state */
- },
- [WIMAX_GNL_STGET_IFIDX] = { .type = NLA_U32, },
- [WIMAX_GNL_MSG_IFIDX] = { .type = NLA_U32, },
- [WIMAX_GNL_MSG_DATA] = {
- .type = NLA_UNSPEC, /* libnl doesn't grok BINARY yet */
- },
+ /*
+ * WIMAX_GNL_RESET_IFIDX, WIMAX_GNL_RFKILL_IFIDX,
+ * WIMAX_GNL_STGET_IFIDX, WIMAX_GNL_MSG_IFIDX
+ */
+ [1] = { .type = NLA_U32, },
+ /*
+ * WIMAX_GNL_RFKILL_STATE, WIMAX_GNL_MSG_PIPE_NAME
+ */
+ [2] = { .type = NLA_U32, }, /* enum wimax_rf_state */
+ /*
+ * WIMAX_GNL_MSG_DATA
+ */
+ [3] = { .type = NLA_UNSPEC, }, /* libnl doesn't grok BINARY yet */
};
static const struct genl_small_ops wimax_gnl_ops[] = {
--
2.27.0
From: Arnd Bergmann <[email protected]>
gcc -Wextra warns about mixing two enum types:
drivers/net/wimax/i2400m/control.c: In function 'i2400m_get_device_info':
drivers/net/wimax/i2400m/control.c:960:10: warning: implicit conversion from 'enum <anonymous>' to 'enum i2400m_tlv' [-Wenum-conversion]
Merge the anonymous enum into the global one that has all the other
values. It's not clear why they were originally kept separate, but this
appears to be the logical place for it.
Fixes: 3a35a1d0bdf7 ("i2400m: various functions for device management")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/net/wimax/i2400m/control.c | 7 -------
include/uapi/linux/wimax/i2400m.h | 1 +
2 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/net/wimax/i2400m/control.c b/drivers/net/wimax/i2400m/control.c
index 8df98757d901..180d5f417bdc 100644
--- a/drivers/net/wimax/i2400m/control.c
+++ b/drivers/net/wimax/i2400m/control.c
@@ -903,13 +903,6 @@ int i2400m_cmd_enter_powersave(struct i2400m *i2400m)
EXPORT_SYMBOL_GPL(i2400m_cmd_enter_powersave);
-/*
- * Definitions for getting device information
- */
-enum {
- I2400M_TLV_DETAILED_DEVICE_INFO = 140
-};
-
/**
* i2400m_get_device_info - Query the device for detailed device information
*
diff --git a/include/uapi/linux/wimax/i2400m.h b/include/uapi/linux/wimax/i2400m.h
index fd198bc24a3c..595ab3511d45 100644
--- a/include/uapi/linux/wimax/i2400m.h
+++ b/include/uapi/linux/wimax/i2400m.h
@@ -409,6 +409,7 @@ enum i2400m_ms {
*/
enum i2400m_tlv {
I2400M_TLV_L4_MESSAGE_VERSIONS = 129,
+ I2400M_TLV_DETAILED_DEVICE_INFO = 140,
I2400M_TLV_SYSTEM_STATE = 141,
I2400M_TLV_MEDIA_STATUS = 161,
I2400M_TLV_RF_OPERATION = 162,
--
2.27.0
From: Arnd Bergmann <[email protected]>
Building with 'make W=2' shows a warning for every time this header
gets included because of a pointer type mismatch:
net/addrconf.h:163:32: warning: passing 'unsigned char *' to parameter of type 'const char *' converts between pointers to integer
types with different sign [-Wpointer-sign]
addrconf_addr_eui48_base(eui, dev->dev_addr);
Change the type to unsigned according to the input argument.
Fixes: 4d6f28591fe4 ("{net,IB}/{rxe,usnic}: Utilize generic mac to eui32 function")
Signed-off-by: Arnd Bergmann <[email protected]>
---
include/net/addrconf.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/net/addrconf.h b/include/net/addrconf.h
index 18f783dcd55f..074ce761e482 100644
--- a/include/net/addrconf.h
+++ b/include/net/addrconf.h
@@ -127,7 +127,8 @@ int addrconf_prefix_rcv_add_addr(struct net *net, struct net_device *dev,
u32 addr_flags, bool sllao, bool tokenized,
__u32 valid_lft, u32 prefered_lft);
-static inline void addrconf_addr_eui48_base(u8 *eui, const char *const addr)
+static inline void addrconf_addr_eui48_base(u8 *eui,
+ const unsigned char *const addr)
{
memcpy(eui, addr, 3);
eui[3] = 0xFF;
@@ -135,7 +136,7 @@ static inline void addrconf_addr_eui48_base(u8 *eui, const char *const addr)
memcpy(eui + 5, addr + 3, 3);
}
-static inline void addrconf_addr_eui48(u8 *eui, const char *const addr)
+static inline void addrconf_addr_eui48(u8 *eui, const unsigned char *const addr)
{
addrconf_addr_eui48_base(eui, addr);
eui[0] ^= 2;
--
2.27.0
From: Arnd Bergmann <[email protected]>
gcc points out an incorrect enum assignment:
drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c: In function 'chcr_ktls_cpl_set_tcb_rpl':
drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c:684:22: warning: implicit conversion from 'enum <anonymous>' to 'enum ch_ktls_open_state' [-Wenum-conversion]
This appears harmless, and should apparently use 'CH_KTLS_OPEN_SUCCESS'
instead of 'false', with the same value '0'.
Fixes: efca3878a5fb ("ch_ktls: Issue if connection offload fails")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
index 5195f692f14d..83787f62577d 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
@@ -681,7 +681,7 @@ static int chcr_ktls_cpl_set_tcb_rpl(struct adapter *adap, unsigned char *input)
kvfree(tx_info);
return 0;
}
- tx_info->open_state = false;
+ tx_info->open_state = CH_KTLS_OPEN_SUCCESS;
spin_unlock(&tx_info->lock);
complete(&tx_info->completion);
--
2.27.0
On Mon, 2020-10-26 at 22:29 +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> gcc -Wextra points out multiple fields that use the same index '1'
> in the wimax_gnl_policy definition:
>
> net/wimax/stack.c:393:29: warning: initialized field overwritten [-Woverride-init]
> net/wimax/stack.c:397:28: warning: initialized field overwritten [-Woverride-init]
> net/wimax/stack.c:398:26: warning: initialized field overwritten [-Woverride-init]
>
> This seems to work since all four use the same NLA_U32 value, but it
> still appears to be wrong. In addition, there is no intializer for
> WIMAX_GNL_MSG_PIPE_NAME, which uses the same index '2' as
> WIMAX_GNL_RFKILL_STATE.
That's funny. This means that WIMAX_GNL_MSG_PIPE_NAME was never used,
since it is meant to be a string, and that won't (usually) fit into 4
bytes...
I suppose that's all an artifact of wimax being completely and utterly
dead anyway. We should probably just remove it.
> Johannes already changed this twice to improve it, but I don't think
> there is a good solution, so try to work around it by using a
> numeric index and adding comments.
Yeah, though maybe there's a better solution now.
Given that we (again and properly) have per-ops policy support, which
really was the thing that broke it here (the commit 3b0f31f2b8c9 you
mentioned), we could split this up into per-ops policies and do the
right thing in the separate policies.
OTOH, that really just makes it use more space, for no discernible
effect to userspace.
So as far as the warning fix is concerned:
Acked-by: Johannes Berg <[email protected]>
Looks like I introduced a bug there with WIMAX_GNL_MSG_PIPE_NAME, but
obviously nobody cared.
johannes
On Tue, Oct 27, 2020 at 5:02 AM Xie He <[email protected]> wrote:
>
> On Mon, Oct 26, 2020 at 8:56 PM Xie He <[email protected]> wrote:
> >
> > > - for (mem = (HDW *) memmap; mem < (HDW *) (memmap + 1); ++mem)
> > > + for (mem = (HDW *) memmap; mem < (HDW *) ((uintptr_t)memmap + 1); ++mem)
> >
> > Note that these two lines are semantically different. In the first line,
> > "+ 1" moves the pointer by (sizeof memmap) bytes. However in the second
> > line, "+ 1" moves the pointer by only 1 byte.
>
> Correction: in the first line "+ 1" moves the pointer by (sizeof *memmap) bytes.
Ah, of course. I had looked up the types but mixed up the memmap
and HDW definitions, but then got confused trying to understand the
logic in wr_mem() that operates on bytes but expands them into
multiples of 4.
I've modified it as below now, will resend along with the other patches
if you think this makes sense.
Arnd
--- a/drivers/atm/horizon.c
+++ b/drivers/atm/horizon.c
@@ -1815,7 +1815,7 @@ static int hrz_init(hrz_dev *dev)
int buff_count;
- HDW * mem;
+ uintptr_t offset;
cell_buf * tx_desc;
cell_buf * rx_desc;
@@ -1841,8 +1841,8 @@ static int hrz_init(hrz_dev *dev)
printk (" clearing memory");
- for (mem = (HDW *) memmap; mem < (HDW *) (memmap + 1); ++mem)
- wr_mem (dev, mem, 0);
+ for (offset = 0; offset < sizeof(struct MEMMAP); offset++)
+ wr_mem (dev, (HDW *)offset, 0);
printk (" tx channels");
> - for (mem = (HDW *) memmap; mem < (HDW *) (memmap + 1); ++mem)
> + for (mem = (HDW *) memmap; mem < (HDW *) ((uintptr_t)memmap + 1); ++mem)
Note that these two lines are semantically different. In the first line,
"+ 1" moves the pointer by (sizeof memmap) bytes. However in the second
line, "+ 1" moves the pointer by only 1 byte.
This driver is old, but let's still keep its code correct!
On Mon, Oct 26, 2020 at 8:56 PM Xie He <[email protected]> wrote:
>
> > - for (mem = (HDW *) memmap; mem < (HDW *) (memmap + 1); ++mem)
> > + for (mem = (HDW *) memmap; mem < (HDW *) ((uintptr_t)memmap + 1); ++mem)
>
> Note that these two lines are semantically different. In the first line,
> "+ 1" moves the pointer by (sizeof memmap) bytes. However in the second
> line, "+ 1" moves the pointer by only 1 byte.
Correction: in the first line "+ 1" moves the pointer by (sizeof *memmap) bytes.
On Tue, Oct 27, 2020 at 8:22 AM Johannes Berg <[email protected]> wrote:
>
> On Mon, 2020-10-26 at 22:29 +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <[email protected]>
> >
> > gcc -Wextra points out multiple fields that use the same index '1'
> > in the wimax_gnl_policy definition:
> >
> > net/wimax/stack.c:393:29: warning: initialized field overwritten [-Woverride-init]
> > net/wimax/stack.c:397:28: warning: initialized field overwritten [-Woverride-init]
> > net/wimax/stack.c:398:26: warning: initialized field overwritten [-Woverride-init]
> >
> > This seems to work since all four use the same NLA_U32 value, but it
> > still appears to be wrong. In addition, there is no intializer for
> > WIMAX_GNL_MSG_PIPE_NAME, which uses the same index '2' as
> > WIMAX_GNL_RFKILL_STATE.
>
> That's funny. This means that WIMAX_GNL_MSG_PIPE_NAME was never used,
> since it is meant to be a string, and that won't (usually) fit into 4
> bytes...
>
> I suppose that's all an artifact of wimax being completely and utterly
> dead anyway. We should probably just remove it.
Makes sense. I checked
https://en.wikipedia.org/wiki/List_of_WiMAX_networks, and it appears
that these entries are all stale, after everyone has migrated to LTE
or discontinued their service altogether.
NetworkManager appears to have dropped userspace support in 2015
https://bugzilla.gnome.org/show_bug.cgi?id=747846, the
http://www.linuxwimax.org site had already shut down earlier.
WiMax is apparently still being deployed on airport campus
networks ("AeroMACS"), but in a frequency band that was not
supported by the old Intel 2400m (used in Sandy Bridge laptops
and earlier), which is the only driver using the kernel's wimax
stack.
Inaky, do you have any additional information about possible
users? If we are sure there are none, then I'd suggest removing
all the wimax code directly, otherwise it could go through
drivers/staging/ for a release or two (and move it back in case
there are users after all). I can send a patch if you like.
> So as far as the warning fix is concerned:
>
> Acked-by: Johannes Berg <[email protected]>
>
Thanks!
Arnd
> From: Arnd Bergmann <[email protected]>
>
> Makes sense. I checked
> https://en.wikipedia.org/wiki/List_of_WiMAX_networks, and it appears
> that these entries are all stale, after everyone has migrated to LTE
> or discontinued their service altogether.
>
> NetworkManager appears to have dropped userspace support in 2015
> https://bugzilla.gnome.org/show_bug.cgi?id=747846, the
> http://www.linuxwimax.org site had already shut down earlier.
>
> WiMax is apparently still being deployed on airport campus
> networks ("AeroMACS"), but in a frequency band that was not
> supported by the old Intel 2400m (used in Sandy Bridge laptops
> and earlier), which is the only driver using the kernel's wimax
> stack.
>
> Inaky, do you have any additional information about possible
> users? If we are sure there are none, then I'd suggest removing
> all the wimax code directly, otherwise it could go through
> drivers/staging/ for a release or two (and move it back in case
> there are users after all). I can send a patch if you like.
I have not
Every now and then I get the occasional message from a student or
researcher asking for support about a production network, but they
have dwindled in the last years.
My vote would be to scrap the whole thing; if there are die hard
users, they can always rise up and move it back from staging.
On Tue, Oct 27, 2020 at 6:24 AM Arnd Bergmann <[email protected]> wrote:
>
> Ah, of course. I had looked up the types but mixed up the memmap
> and HDW definitions, but then got confused trying to understand the
> logic in wr_mem() that operates on bytes but expands them into
> multiples of 4.
I think wr_mem() doesn't try to expand the address into multiples of
4. The address is multiplied by "sizeof(HDW)", which is 1. So the
address is not expanded.
I think this driver uses 0-based pointers not as byte-addresses to
access the host memory, but as (32-bit) word-addresses to access the
special hardware address space.
So using pointers in this case is confusing because it makes people
incorrectly consider they are used to access the host memory. It'd be
better that we just use integers.
> I've modified it as below now, will resend along with the other patches
> if you think this makes sense.
>
> Arnd
>
> --- a/drivers/atm/horizon.c
> +++ b/drivers/atm/horizon.c
> @@ -1815,7 +1815,7 @@ static int hrz_init(hrz_dev *dev)
>
> int buff_count;
>
> - HDW * mem;
> + uintptr_t offset;
>
> cell_buf * tx_desc;
> cell_buf * rx_desc;
> @@ -1841,8 +1841,8 @@ static int hrz_init(hrz_dev *dev)
>
> printk (" clearing memory");
>
> - for (mem = (HDW *) memmap; mem < (HDW *) (memmap + 1); ++mem)
> - wr_mem (dev, mem, 0);
> + for (offset = 0; offset < sizeof(struct MEMMAP); offset++)
> + wr_mem (dev, (HDW *)offset, 0);
>
> printk (" tx channels");
This looks good to me. Thanks!
On Mon, 26 Oct 2020 22:29:48 +0100 Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> Building a "W=1" kernel with clang produces a warning about
> suspicous pointer arithmetic:
>
> drivers/atm/horizon.c:1844:52: warning: performing pointer arithmetic
> on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
> for (mem = (HDW *) memmap; mem < (HDW *) (memmap + 1); ++mem)
>
> The way that the addresses are handled is very obscure, and
> rewriting it to be more conventional seems fairly pointless, given
> that this driver probably has no users.
> Shut up this warning by adding a cast to uintptr_t.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Arnd Bergmann <[email protected]>
Hi!
I'm not sure what your plan is for re-spinning but when you do could
you please split the wireless changes out? Also we never got patch 3
IDK if that's a coincidence or if it wasn't for networking...
On Wed, Oct 28, 2020 at 1:42 AM Jakub Kicinski <[email protected]> wrote:
>
> On Mon, 26 Oct 2020 22:29:48 +0100 Arnd Bergmann wrote:
> > From: Arnd Bergmann <[email protected]>
> >
> > Building a "W=1" kernel with clang produces a warning about
> > suspicous pointer arithmetic:
> >
> > drivers/atm/horizon.c:1844:52: warning: performing pointer arithmetic
> > on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
> > for (mem = (HDW *) memmap; mem < (HDW *) (memmap + 1); ++mem)
> >
> > The way that the addresses are handled is very obscure, and
> > rewriting it to be more conventional seems fairly pointless, given
> > that this driver probably has no users.
> > Shut up this warning by adding a cast to uintptr_t.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Arnd Bergmann <[email protected]>
>
> Hi!
>
> I'm not sure what your plan is for re-spinning but when you do could
> you please split the wireless changes out?
Sure, will do. The easiest for me would be if you just merge the ones
that have been acked or that look obvious enough for you, and I'll
then resend whatever is left after addressing the review comments.
If that causes you extra work, I'll just send everything that should go
through your tree.
> Also we never got patch 3
> IDK if that's a coincidence or if it wasn't for networking...
Yes, that one slipped in when I was sorting my longer series, it
was a block driver change.
Arnd
On Mon, Oct 26, 2020 at 2:31 PM Arnd Bergmann <[email protected]> wrote:
>
> From: Arnd Bergmann <[email protected]>
>
> The returned string from rsxx_card_state_to_str is 'const',
> but the other qualifier doesn't change anything here except
> causing a warning with 'clang -Wextra':
>
> drivers/block/rsxx/core.c:393:21: warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers]
> static const char * const rsxx_card_state_to_str(unsigned int state)
>
> Fixes: f37912039eb0 ("block: IBM RamSan 70/80 trivial changes.")
> Signed-off-by: Arnd Bergmann <[email protected]>
Reviewed-by: Nick Desaulniers <[email protected]>
> ---
> drivers/block/rsxx/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/block/rsxx/core.c b/drivers/block/rsxx/core.c
> index 63f549889f87..d0af46d7b681 100644
> --- a/drivers/block/rsxx/core.c
> +++ b/drivers/block/rsxx/core.c
> @@ -390,7 +390,7 @@ static irqreturn_t rsxx_isr(int irq, void *pdata)
> }
>
> /*----------------- Card Event Handler -------------------*/
> -static const char * const rsxx_card_state_to_str(unsigned int state)
> +static const char *rsxx_card_state_to_str(unsigned int state)
> {
> static const char * const state_strings[] = {
> "Unknown", "Shutdown", "Starting", "Formatting",
> --
> 2.27.0
>
--
Thanks,
~Nick Desaulniers
On Thu, 2020-10-29 at 12:34 -0700, Nick Desaulniers wrote:
> On Mon, Oct 26, 2020 at 2:31 PM Arnd Bergmann <[email protected]> wrote:
> >
> > From: Arnd Bergmann <[email protected]>
> >
> > The returned string from rsxx_card_state_to_str is 'const',
> > but the other qualifier doesn't change anything here except
> > causing a warning with 'clang -Wextra':
> >
> > drivers/block/rsxx/core.c:393:21: warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers]
> > static const char * const rsxx_card_state_to_str(unsigned int state)
> >
> > Fixes: f37912039eb0 ("block: IBM RamSan 70/80 trivial changes.")
> > Signed-off-by: Arnd Bergmann <[email protected]>
>
> Reviewed-by: Nick Desaulniers <[email protected]>
Perhaps this should also be converted to avoid any possible
dereference of strings with an invalid state.
---
drivers/block/rsxx/core.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/block/rsxx/core.c b/drivers/block/rsxx/core.c
index 8799e3bab067..f50b00b4887f 100644
--- a/drivers/block/rsxx/core.c
+++ b/drivers/block/rsxx/core.c
@@ -390,15 +390,27 @@ static irqreturn_t rsxx_isr(int irq, void *pdata)
}
/*----------------- Card Event Handler -------------------*/
-static const char * const rsxx_card_state_to_str(unsigned int state)
+static const char *rsxx_card_state_to_str(unsigned int state)
{
static const char * const state_strings[] = {
- "Unknown", "Shutdown", "Starting", "Formatting",
- "Uninitialized", "Good", "Shutting Down",
- "Fault", "Read Only Fault", "dStroying"
+ "Unknown", /* no bit set - all zeros */
+ "Shutdown", /* BIT(0) */
+ "Starting", /* BIT(1) */
+ "Formatting", /* BIT(2) */
+ "Uninitialized", /* BIT(3) */
+ "Good", /* BIT(4) */
+ "Shutting Down", /* BIT(5) */
+ "Fault", /* BIT(6) */
+ "Read Only Fault", /* BIT(7) */
+ "Destroying" /* BIT(8) */
};
- return state_strings[ffs(state)];
+ int i = ffs(state);
+
+ if (i >= ARRAY_SIZE(state_strings))
+ return "Invalid state";
+
+ return state_strings[i];
}
static void card_state_change(struct rsxx_cardinfo *card,