LinuxLists.cc - [PATCH] wireless: at76c50x: fix multithread access to hex2str

2011-09-28 13:18:00

Subject: [PATCH] wireless: at76c50x: fix multithread access to hex2str

The original hex2str uses finite array of buffers to keep output data. It's a wrong approach, because we can't say at compile time how many threads will be used.

This patch introduces one buffer and global mutex to access this function. All
calls of it are wrapped by locking this mutex. It saves some memory as a side
effect.

Signed-off-by: Andy Shevchenko <[email protected]>
Cc: "John W. Linville" <[email protected]>
---
drivers/net/wireless/at76c50x-usb.c | 23 +++++++++++++++++++----
1 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
index 39322d4..af043f6 100644
--- a/drivers/net/wireless/at76c50x-usb.c
+++ b/drivers/net/wireless/at76c50x-usb.c
@@ -498,15 +498,14 @@ exit:
return ret;
}

-#define HEX2STR_BUFFERS 4
#define HEX2STR_MAX_LEN 64

+static DEFINE_MUTEX(hex2str_mutex);
+
/* Convert binary data into hex string */
static char *hex2str(void *buf, size_t len)
{
- static atomic_t a = ATOMIC_INIT(0);
- static char bufs[HEX2STR_BUFFERS][3 * HEX2STR_MAX_LEN + 1];
- char *ret = bufs[atomic_inc_return(&a) & (HEX2STR_BUFFERS - 1)];
+ static char ret[3 * HEX2STR_MAX_LEN + 1];
char *obuf = ret;
u8 *ibuf = buf;

@@ -1003,10 +1002,13 @@ static void at76_dump_mib_mac_wep(struct at76_priv *priv)
key_len = (m->encryption_level == 1) ?
WEP_SMALL_KEY_LEN : WEP_LARGE_KEY_LEN;

+ mutex_lock(&hex2str_mutex);
for (i = 0; i < WEP_KEYS; i++)
at76_dbg(DBG_MIB, "%s: MIB MAC_WEP: key %d: %s",
wiphy_name(priv->hw->wiphy), i,
hex2str(m->wep_default_keyvalue[i], key_len));
+ mutex_unlock(&hex2str_mutex);
+
exit:
kfree(m);
}
@@ -1028,6 +1030,7 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
goto exit;
}

+ mutex_lock(&hex2str_mutex);
at76_dbg(DBG_MIB, "%s: MIB MAC_MGMT: beacon_period %d CFP_max_duration "
"%d medium_occupancy_limit %d station_id 0x%x ATIM_window %d "
"CFP_mode %d privacy_opt_impl %d DTIM_period %d CFP_period %d "
@@ -1045,6 +1048,8 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
m->current_bss_type, m->power_mgmt_mode, m->ibss_change,
m->res, m->multi_domain_capability_implemented,
m->multi_domain_capability_enabled, m->country_string);
+ mutex_unlock(&hex2str_mutex);
+
exit:
kfree(m);
}
@@ -1064,6 +1069,7 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
goto exit;
}

+ mutex_lock(&hex2str_mutex);
at76_dbg(DBG_MIB, "%s: MIB MAC: max_tx_msdu_lifetime %d "
"max_rx_lifetime %d frag_threshold %d rts_threshold %d "
"cwmin %d cwmax %d short_retry_time %d long_retry_time %d "
@@ -1082,6 +1088,8 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
le16_to_cpu(m->listen_interval),
hex2str(m->desired_ssid, IW_ESSID_MAX_SIZE),
m->desired_bssid, m->desired_bsstype);
+ mutex_unlock(&hex2str_mutex);
+
exit:
kfree(m);
}
@@ -1160,6 +1168,8 @@ static void at76_dump_mib_mdomain(struct at76_priv *priv)
goto exit;
}

+ mutex_lock(&hex2str_mutex);
+
at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: channel_list %s",
wiphy_name(priv->hw->wiphy),
hex2str(m->channel_list, sizeof(m->channel_list)));
@@ -1167,6 +1177,8 @@ static void at76_dump_mib_mdomain(struct at76_priv *priv)
at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: tx_powerlevel %s",
wiphy_name(priv->hw->wiphy),
hex2str(m->tx_powerlevel, sizeof(m->tx_powerlevel)));
+
+ mutex_unlock(&hex2str_mutex);
exit:
kfree(m);
}
@@ -1368,6 +1380,7 @@ static int at76_startup_device(struct at76_priv *priv)
struct at76_card_config *ccfg = &priv->card_config;
int ret;

+ mutex_lock(&hex2str_mutex);
at76_dbg(DBG_PARAMS,
"%s param: ssid %.*s (%s) mode %s ch %d wep %s key %d "
"keylen %d", wiphy_name(priv->hw->wiphy), priv->essid_size,
@@ -1375,6 +1388,8 @@ static int at76_startup_device(struct at76_priv *priv)
priv->iw_mode == IW_MODE_ADHOC ? "adhoc" : "infra",
priv->channel, priv->wep_enabled ? "enabled" : "disabled",
priv->wep_key_id, priv->wep_keys_len[priv->wep_key_id]);
+ mutex_unlock(&hex2str_mutex);
+
at76_dbg(DBG_PARAMS,
"%s param: preamble %s rts %d retry %d frag %d "
"txrate %s auth_mode %d", wiphy_name(priv->hw->wiphy),
--
1.7.6.3

2011-09-30 15:53:41

by Pavel Roskin

[permalink] [raw]

Subject: Re: [PATCH] wireless: at76c50x: fix multithread access to hex2str

On Thu, 29 Sep 2011 13:46:55 +0300
Andy Shevchenko <[email protected]> wrote:

> On Wed, 2011-09-28 at 17:19 -0400, Pavel Roskin wrote:
> > Or maybe you could develop an extension for printk() format that
> > would dump strings of the given length? Something like %pM, but
> > with an extra argument (and make sure it would not trigger a gcc
> > warning). This way, everybody would benefit.
> I don't think it would be a noticeable benefit. On the other hand
> vsnprintf() is hugely overloaded by many "extensions" to the C99
> variant.

I looked at the code, and it seems very little is needed to support
"%*pM"

> > Please rethink whether it's helpful to send such "fixes" for old and
> > little maintained drivers.
> Last copyright is 2010, TODO list actually suggests to remove hex2str
> at all.

I don't know where it's written, but I think it's a good idea,
especially if there is a better alternative available for all drivers.

--
Regards,
Pavel Roskin

2011-09-28 21:19:19

by Pavel Roskin

[permalink] [raw]

Subject: Re: [PATCH] wireless: at76c50x: fix multithread access to hex2str

On Wed, 28 Sep 2011 16:17:31 +0300
Andy Shevchenko <[email protected]> wrote:

> The original hex2str uses finite array of buffers to keep output
> data. It's a wrong approach, because we can't say at compile time how
> many threads will be used.
>
> This patch introduces one buffer and global mutex to access this
> function. All calls of it are wrapped by locking this mutex. It saves
> some memory as a side effect.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> Cc: "John W. Linville" <[email protected]>

The reason for having 4 buffers is to allow using hex2str() more than
once in the same printk() statement. As you can see, hex2str() is
never used in one statement more than once. Your solution would make
it impossible to use hex2str() twice in the same printk(), as the
buffer would be reused. Actually, wrong data would be printed with no
warning! Such code doesn't exist in the driver, but it could be added
by somebody looking for a problem. And that would hamper debugging
instead of helping it.

I think we can live with a debug function printing wrong data if it's
called by 5 callers at once.

And if you want a perfectly correct fix, I suggest that you use
buffers allocated by the callers on the stack. That's less intrusive
than mutexes, and it would allow using more than one hex2str() in one
statement as long as the buffers are different.

Or maybe hex2str() should be replaced with calls to
print_hex_dump_bytes()? That would add extra lines to the kernel
output, but it's no big deal. Of course, if you are concerned about 5
callers printing bytes in the same time, the output may be hard to
understand, but I don't think it a problem that needs addressing.

Or maybe you could develop an extension for printk() format that would
dump strings of the given length? Something like %pM, but with an
extra argument (and make sure it would not trigger a gcc warning).
This way, everybody would benefit.

Please rethink whether it's helpful to send such "fixes" for old and
little maintained drivers. There are few people who can work on them,
and they have little time for the old drivers. Their time is better
spent fixing real problems than reviewing untested or badly conceived
patches for imaginary problems and style issues. Besides, there are few
users who could test the driver and report breakage.

--
Regards,
Pavel Roskin

2011-09-29 10:47:40

Subject: [PATCH] wireless: at76c50x: fix multithread access to hex2str

Subject: Re: [PATCH] wireless: at76c50x: fix multithread access to hex2str

Subject: Re: [PATCH] wireless: at76c50x: fix multithread access to hex2str

Subject: Re: [PATCH] wireless: at76c50x: fix multithread access to hex2str

Subject: Re: [RFC][PATCH 1/2] lib: printf: append support of '%*p[Mm][FR]'

Subject: [RFC][PATCH 2/2] wireless: at76c50x: eliminate hex2str()

Subject: Re: [RFC][PATCH 1/2] lib: printf: append support of '%*p[Mm][FR]'

Subject: Re: [RFC][PATCH 2/2] wireless: at76c50x: eliminate hex2str()

Subject: [RFC][PATCH 1/2] lib: printf: append support of '%*p[Mm][FR]'

Subject: Re: [RFC][PATCH 1/2] lib: printf: append support of '%*p[Mm][FR]'

Subject: Re: [RFC][PATCHv2 2/3] lib: printf: append support of '%*p[Mm][FR]'

Subject: Re: [PATCHv3 1/3] lib: printf: update documentation to cover all of %p[Mm][FR]

Subject: Re: [PATCHv3 2/3] lib: printf: append support of '%*ph[CDN]'

Subject: Re: [PATCHv3 2/3] lib: printf: append support of '%*ph[CDN]'

Subject: [RFC][PATCHv2 2/3] lib: printf: append support of '%*p[Mm][FR]'

Subject: Re: [RFC][PATCHv2 2/3] lib: printf: append support of '%*p[Mm][FR]'

Subject: Re: [RFC][PATCHv2 3/3] wireless: at76c50x: eliminate hex2str()

Subject: Re: [RFC][PATCHv2 3/3] wireless: at76c50x: eliminate hex2str()

Subject: Re: [RFC][PATCHv2 2/3] lib: printf: append support of '%*p[Mm][FR]'

Subject: Re: [RFC][PATCH 1/2] lib: printf: append support of '%*p[Mm][FR]'

Subject: [RFC][PATCHv2 3/3] wireless: at76c50x: eliminate hex2str()

Subject: Re: [RFC][PATCHv2 2/3] lib: printf: append support of '%*p[Mm][FR]'

Subject: [PATCHv3 2/3] lib: printf: append support of '%*ph[CDN]'

Subject: [PATCHv3 1/3] lib: printf: update documentation to cover all of %p[Mm][FR]

Subject: [PATCHv3.6] lib: printf: append support of '%*ph[CDN]'

Subject: [PATCHv3 3/3] wireless: at76c50x: eliminate hex2str()

Subject: Re: [RFC][PATCH 1/2] lib: printf: append support of '%*p[Mm][FR]'

Subject: [RFC][PATCHv2 1/3] lib: printf: update documentation to cover all of %p[Mm][FR]

Subject: [resend][PATCH] wireless: at76c50x: eliminate hex2str()

Subject: Re: [resend][PATCH] wireless: at76c50x: eliminate hex2str()

Subject: Re: [resend][PATCH] wireless: at76c50x: eliminate hex2str()