[PATCH] net/wireless/bcom: constify ieee80211_get_response_rate return
It's not modified so make it const with the eventual goal of moving
data to text for various static struct ieee80211_rate arrays.
Signed-off-by: Joe Perches
---
drivers/net/wireless/broadcom/b43/main.c | 2 +-
drivers/net/wireless/broadcom/b43legacy/main.c | 2 +-
include/net/cfg80211.h | 2 +-
net/wireless/util.c| 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/broadcom/b43/main.c
b/drivers/net/wireless/broadcom/b43/main.c
index 150a366e8f62..17bcec5f3ff7 100644
--- a/drivers/net/wireless/broadcom/b43/main.c
+++ b/drivers/net/wireless/broadcom/b43/main.c
@@ -4053,7 +4053,7 @@ static void b43_update_basic_rates(struct b43_wldev *dev,
u32 brates)
{
struct ieee80211_supported_band *sband =
dev->wl->hw->wiphy->bands[b43_current_band(dev->wl)];
- struct ieee80211_rate *rate;
+ const struct ieee80211_rate *rate;
int i;
u16 basic, direct, offset, basic_offset, rateptr;
diff --git a/drivers/net/wireless/broadcom/b43legacy/main.c
b/drivers/net/wireless/broadcom/b43legacy/main.c
index 7692a2618c97..f64ebff68308 100644
--- a/drivers/net/wireless/broadcom/b43legacy/main.c
+++ b/drivers/net/wireless/broadcom/b43legacy/main.c
@@ -2762,7 +2762,7 @@ static void b43legacy_update_basic_rates(struct
b43legacy_wldev *dev, u32 brates
{
struct ieee80211_supported_band *sband =
dev->wl->hw->wiphy->bands[NL80211_BAND_2GHZ];
- struct ieee80211_rate *rate;
+ const struct ieee80211_rate *rate;
int i;
u16 basic, direct, offset, basic_offset, rateptr;
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 911fae42b0c0..ed07590bc72d 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5606,7 +5606,7 @@ static inline bool cfg80211_channel_is_psc(struct
ieee80211_channel *chan)
* which is, for this function, given as a bitmap of indices of
* rates in the band's bitrate table.
*/
-struct ieee80211_rate *
+const struct ieee80211_rate *
ieee80211_get_response_rate(struct ieee80211_supported_band *sband,
u32 basic_rates, int bitrate);
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 1bf0200f562a..382c5262d997 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -24,7 +24,7 @@
#include "rdev-ops.h"
-struct ieee80211_rate *
+const struct ieee80211_rate *
ieee80211_get_response_rate(struct ieee80211_supported_band *sband,
u32 basic_rates, int bitrate)
{
Re: [PATCH] brcmsmac: fix shift on 4 bit masked value
On Sun, 2021-04-18 at 06:10 +, Kalle Valo wrote:
> Colin King wrote:
>
> > From: Colin Ian King
> >
> > The calculation of offtune_val seems incorrect, the u16 value in
> > pi->tx_rx_cal_radio_saveregs[2] is being masked with 0xf0 and then
> > shifted 8 places right so that always ends up as a zero result. I
> > believe the intended shift was 4 bits to the right. Fix this.
> >
> > [Note: not tested, I don't have the H/W]
> >
> > Addresses-Coverity: ("Operands don't affect result")
> > Fixes: 5b435de0d786 ("net: wireless: add brcm80211 drivers")
> > Signed-off-by: Colin Ian King
>
> I think this needs review from someone familiar with the hardware.
>
> Patch set to Changes Requested.
What "change" are you requesting here?
Likely there needs to be some other setting for the patch.
Perhaps "deferred" as you seem to be requesting a review
and there's no actual change necessary, just approval from
someone with the hardware and that someone test the patch.
Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
On Sat, 2021-04-17 at 14:30 -0400, Jes Sorensen wrote: > On 4/17/21 1:52 PM, Kalle Valo wrote: > > "Gustavo A. R. Silva" wrote: > > > > > In preparation to enable -Wimplicit-fallthrough for Clang, fix > > > multiple warnings by replacing /* fall through */ comments with > > > the new pseudo-keyword macro fallthrough; instead of letting the > > > code fall through to the next case. > > > > > > Notice that Clang doesn't recognize /* fall through */ comments as > > > implicit fall-through markings. > > > > > > Link: https://github.com/KSPP/linux/issues/115 > > > Signed-off-by: Gustavo A. R. Silva > > > > Patch applied to wireless-drivers-next.git, thanks. > > > > bf3365a856a1 rtl8xxxu: Fix fall-through warnings for Clang > > > > Sorry this junk patch should not have been applied. I don't believe it's a junk patch. I believe your characterization of it as such is flawed. You don't like the style, that's fine, but: Any code in the kernel should not be a unique style of your own choosing when it could cause various compilers to emit unnecessary warnings. Please remember the kernel code base is a formed by a community with a nominally generally accepted style. There is a real desire in that community to both enable compiler warnings that might show defects and simultaneously avoid unnecessary compiler warnings. This particular change just avoids a possible compiler warning.
Re: [PATCH][next] mlxsw: spectrum_router: remove redundant initialization of variable force
On Mon, 2021-03-29 at 09:04 +0300, Dan Carpenter wrote:
> On Sat, Mar 27, 2021 at 10:33:34PM +, Colin King wrote:
> > From: Colin Ian King
> >
> > The variable force is being initialized with a value that is
> > never read and it is being updated later with a new value. The
> > initialization is redundant and can be removed.
> >
> > Addresses-Coverity: ("Unused value")
> > Signed-off-by: Colin Ian King
> > ---
> > drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > index 6ccaa194733b..ff240e3c29f7 100644
> > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > @@ -5059,7 +5059,7 @@ mlxsw_sp_nexthop_obj_bucket_adj_update(struct
> > mlxsw_sp *mlxsw_sp,
> > {
> > u16 bucket_index = info->nh_res_bucket->bucket_index;
> > struct netlink_ext_ack *extack = info->extack;
> > - bool force = info->nh_res_bucket->force;
> > + bool force;
> > char ratr_pl_new[MLXSW_REG_RATR_LEN];
> > char ratr_pl[MLXSW_REG_RATR_LEN];
> > u32 adj_index;
>
> Reverse Christmas tree.
Is still terrible style.
Re: [PATCH] Bluetooth: af_bluetooth: checkpatch: fix indentation and alignment
On Thu, 2021-01-28 at 00:05 +0900, Tomoyuki Matsushita wrote: > Signed-off-by: Tomoyuki Matsushita checkpatch is pretty stupid so whatever it recommends needs to be looked at carefully by a human and fixed appropriately. > diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c [] > @@ -478,7 +478,7 @@ __poll_t bt_sock_poll(struct file *file, struct socket > *sock, > mask |= EPOLLHUP; > > > if (sk->sk_state == BT_CONNECT || > - sk->sk_state == BT_CONNECT2 || > + sk->sk_state == BT_CONNECT2 || > sk->sk_state == BT_CONFIG) checkpatch only warns on the alignment of the second line of multi-line statements. Please make sure all lines of the multi-line statements are aligned.
Re: [PATCH v3] rtlwifi: Simplify bool comparison
On Tue, 2021-01-26 at 16:31 +0800, Jiapeng Zhong wrote:
> Fix the following coccicheck warning:
> ./drivers/net/wireless/realtek/rtlwifi/ps.c:798:7-21: WARNING:
> Comparison to bool
> ./drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c:3848:7-17:
> WARNING: Comparison of 0/1 to bool variable
[]
> diff --git a/drivers/net/wireless/realtek/rtlwifi/ps.c
> b/drivers/net/wireless/realtek/rtlwifi/ps.c
[]
> @@ -798,9 +798,9 @@ static void rtl_p2p_noa_ie(struct ieee80211_hw *hw, void
> *data,
> ie += 3 + noa_len;
> }
>
>
> - if (find_p2p_ie == true) {
> + if (find_p2p_ie) {
> if ((p2pinfo->p2p_ps_mode > P2P_PS_NONE) &&
> - (find_p2p_ps_ie == false))
> + (!find_p2p_ps_ie))
> rtl_p2p_ps_cmd(hw, P2P_PS_DISABLE);
> }
Always review suggested coccinelle patches before submission and
see if there are ways to improve the code beyond what the spatch
tool suggests.
Perhaps integrate these tests and removed an indent level too:
if (find_p2p_ie && !find_p2p_ps_ie &&
p2pinfo->p2p_ps_mode > P2P_PS_NONE)
rtl_p2p_ps_cmd(hw, P2P_PS_DISABLE);
Re: [PATCH net-next 07/15] bnxt_en: log firmware debug notifications
On Mon, 2021-01-25 at 02:08 -0500, Michael Chan wrote:
> From: Edwin Peer
>
> Firmware is capable of generating asynchronous debug notifications.
> The event data is opaque to the driver and is simply logged. Debug
> notifications can be enabled by turning on hardware status messages
> using the ethtool msglvl interface.
[]
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
[]
> @@ -2072,6 +2073,13 @@ static int bnxt_async_event_process(struct bnxt *bp,
> bnxt_fw_health_readl(bp, BNXT_FW_RESET_CNT_REG);
> goto async_event_process_exit;
> }
> + case ASYNC_EVENT_CMPL_EVENT_ID_DEBUG_NOTIFICATION:
> + if (netif_msg_hw(bp)) {
> + netdev_notice(bp->dev,
> + "Received firmware debug notification,
> data1: 0x%x, data2: 0x%x\n",
> + data1, data2);
> + }
netif_notice(bp, hw, bp->dev,
"Received firmware debug notification, data1:
0x%x, data2: 0x%x\n",
data1, data2);
> + goto async_event_process_exit;
> case ASYNC_EVENT_CMPL_EVENT_ID_RING_MONITOR_MSG: {
> struct bnxt_rx_ring_info *rxr;
> u16 grp_idx;
Re: [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs
On Tue, 2021-01-26 at 10:48 +0200, Leon Romanovsky wrote:
> On Tue, Jan 26, 2021 at 12:20:11AM -0800, Joe Perches wrote:
> > On Tue, 2021-01-26 at 08:01 +0200, Leon Romanovsky wrote:
> > > On Mon, Jan 25, 2021 at 01:52:29PM -0800, Jakub Kicinski wrote:
> > > > On Sun, 24 Jan 2021 15:11:16 +0200 Leon Romanovsky wrote:
> > > > > +static int pci_enable_vfs_overlay(struct pci_dev *dev) { return 0; }
> > > > > +static void pci_disable_vfs_overlay(struct pci_dev *dev) {}
[]
> > $ ./scripts/checkpatch.pl -f include/linux/*.h --types=static_inline
> > --terse --nosummary
> > include/linux/dma-mapping.h:203: WARNING: static function definition might
> > be better as static inline
> > include/linux/genl_magic_func.h:55: WARNING: static function definition
> > might be better as static inline
> > include/linux/genl_magic_func.h:78: WARNING: static function definition
> > might be better as static inline
> > include/linux/kernel.h:670: WARNING: static function definition might be
> > better as static inline
> > include/linux/kprobes.h:213: WARNING: static function definition might be
> > better as static inline
> > include/linux/kprobes.h:231: WARNING: static function definition might be
> > better as static inline
> > include/linux/kprobes.h:511: WARNING: static function definition might be
> > better as static inline
> > include/linux/skb_array.h:185: WARNING: static function definition might be
> > better as static inline
> > include/linux/slab.h:606: WARNING: static function definition might be
> > better as static inline
> > include/linux/stop_machine.h:62: WARNING: static function definition might
> > be better as static inline
> > include/linux/vmw_vmci_defs.h:850: WARNING: static function definition
> > might be better as static inline
> > include/linux/zstd.h:95: WARNING: static function definition might be
> > better as static inline
> > include/linux/zstd.h:106: WARNING: static function definition might be
> > better as static inline
> >
> > A false positive exists when __must_check is used between
> > static and inline. It's an unusual and IMO not a preferred use.
>
> Maybe just filter and ignore such functions for now?
Not worth it.
> Will you send proper patch or do you want me to do it?
I'll do it eventually.
Re: [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs
On Tue, 2021-01-26 at 08:01 +0200, Leon Romanovsky wrote:
> On Mon, Jan 25, 2021 at 01:52:29PM -0800, Jakub Kicinski wrote:
> > On Sun, 24 Jan 2021 15:11:16 +0200 Leon Romanovsky wrote:
> > > +static int pci_enable_vfs_overlay(struct pci_dev *dev) { return 0; }
> > > +static void pci_disable_vfs_overlay(struct pci_dev *dev) {}
> >
> > s/static /static inline /
>
> Thanks a lot, I think that we should extend checkpatch.pl to catch such
> mistakes.
Who is this "we" you refer to? ;)
> How hard is it to extend checkpatch.pl to do regexp and warn if in *.h file
> someone declared function with implementation but didn't add "inline" word?
Something like this seems reasonable and catches these instances in
include/linux/*.h
$ ./scripts/checkpatch.pl -f include/linux/*.h --types=static_inline --terse
--nosummary
include/linux/dma-mapping.h:203: WARNING: static function definition might be
better as static inline
include/linux/genl_magic_func.h:55: WARNING: static function definition might
be better as static inline
include/linux/genl_magic_func.h:78: WARNING: static function definition might
be better as static inline
include/linux/kernel.h:670: WARNING: static function definition might be better
as static inline
include/linux/kprobes.h:213: WARNING: static function definition might be
better as static inline
include/linux/kprobes.h:231: WARNING: static function definition might be
better as static inline
include/linux/kprobes.h:511: WARNING: static function definition might be
better as static inline
include/linux/skb_array.h:185: WARNING: static function definition might be
better as static inline
include/linux/slab.h:606: WARNING: static function definition might be better
as static inline
include/linux/stop_machine.h:62: WARNING: static function definition might be
better as static inline
include/linux/vmw_vmci_defs.h:850: WARNING: static function definition might be
better as static inline
include/linux/zstd.h:95: WARNING: static function definition might be better as
static inline
include/linux/zstd.h:106: WARNING: static function definition might be better
as static inline
A false positive exists when __must_check is used between
static and inline. It's an unusual and IMO not a preferred use.
---
scripts/checkpatch.pl | 12
1 file changed, 12 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4f8494527139..0ac366481962 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4451,6 +4451,18 @@ sub process {
}
}
+# check for static function definitions without inline in .h files
+# only works for static in column 1 and avoids multiline macro definitions
+ if ($realfile =~ /\.h$/ &&
+ defined($stat) &&
+ $stat =~
/^\+static(?!\s+(?:$Inline|union|struct))\b.*\{.*\}\s*$/s &&
+ $line =~ /^\+static(?!\s+(?:$Inline|union|struct))\b/ &&
+ $line !~ /\\$/) {
+ WARN("STATIC_INLINE",
+"static function definition might be better as
static inline\n" .
+ $herecurr);
+ }
+
# check for non-global char *foo[] = {"bar", ...} declarations.
if ($line =~
/^.\s+(?:static\s+|const\s+)?char\s+\*\s*\w+\s*\[\s*\]\s*=\s*\{/) {
WARN("STATIC_CONST_CHAR_ARRAY",
[PATCH] checkpatch: Add kmalloc_array_node to unnecessary OOM message check
commit 5799b255c491 ("include/linux/slab.h: add kmalloc_array_node() and
kcalloc_node()") was added in 2017. Update the unnecessary OOM message
test to include it.
Signed-off-by: Joe Perches
Reported-by: Jakub Kicinski
---
Maybe not worth fixing, but no real effort to fix either.
scripts/checkpatch.pl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4f8494527139..8dbf1986a8de 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -487,7 +487,7 @@ our $logFunctions = qr{(?x:
our $allocFunctions = qr{(?x:
(?:(?:devm_)?
- (?:kv|k|v)[czm]alloc(?:_node|_array)? |
+ (?:kv|k|v)[czm]alloc(?:_array)?(?:_node)? |
kstrdup(?:_const)? |
kmemdup(?:_nul)?) |
(?:\w+)?alloc_skb(?:_ip_align)? |
Re: [PATCH] arcnet: fix macro name when DEBUG is defined
On Sun, 2021-01-17 at 10:15 -0800, t...@redhat.com wrote:
> From: Tom Rix
>
> When DEBUG is defined this error occurs
>
> drivers/net/arcnet/com20020_cs.c:70:15: error: ‘com20020_REG_W_ADDR_HI’
> undeclared (first use in this function);
> did you mean ‘COM20020_REG_W_ADDR_HI’?
> ioaddr, com20020_REG_W_ADDR_HI);
> ^~
>
> From reviewing the context, the suggestion is what is meant.
>
> Fixes: 0fec65130b9f ("arcnet: com20020: Use arcnet_ routines")
Nice find thanks, especially seeing as how this hasn't been tested or
compiled in 5+ years...
commit 0fec65130b9f11a73d74f47025491f97f82ba070
Author: Joe Perches
Date: Tue May 5 10:06:06 2015 -0700
Acked-by: Joe Perches
> Signed-off-by: Tom Rix
> ---
> drivers/net/arcnet/com20020_cs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/arcnet/com20020_cs.c
> b/drivers/net/arcnet/com20020_cs.c
> index cf607ffcf358..81223f6bebcc 100644
> --- a/drivers/net/arcnet/com20020_cs.c
> +++ b/drivers/net/arcnet/com20020_cs.c
> @@ -67,7 +67,7 @@ static void regdump(struct net_device *dev)
> /* set up the address register */
> count = 0;
> arcnet_outb((count >> 8) | RDDATAflag | AUTOINCflag,
> - ioaddr, com20020_REG_W_ADDR_HI);
> + ioaddr, COM20020_REG_W_ADDR_HI);
> arcnet_outb(count & 0xff, ioaddr, COM20020_REG_W_ADDR_LO);
>
>
> for (count = 0; count < 256 + 32; count++) {
Re: [Intel-wired-lan] [net-next] net: iavf: Use the ARRAY_SIZE macro for aq_to_posix
On Thu, 2021-01-14 at 09:57 +, Jankowski, Konrad0 wrote: > > -Original Message- > > From: Intel-wired-lan On Behalf Of Wei > > Xu [] > > Use the ARRAY_SIZE macro to calculate the size of an array. > > This code was detected with the help of Coccinelle. [] > > diff --git a/drivers/net/ethernet/intel/iavf/iavf_adminq.h [] > > @@ -120,7 +120,7 @@ static inline int iavf_aq_rc_to_posix(int aq_ret, int > > aq_rc) > > if (aq_ret == IAVF_ERR_ADMIN_QUEUE_TIMEOUT) > > return -EAGAIN; > > > > - if (!((u32)aq_rc < (sizeof(aq_to_posix) / sizeof((aq_to_posix)[0] > > + if (!((u32)aq_rc < ARRAY_SIZE(aq_to_posix))) > > return -ERANGE; > > > > return aq_to_posix[aq_rc]; > > Tested-by: Konrad Jankowski I think several things are poor here. This function shouldn't really be a static inline as it would just bloat whatever uses it and should just be a typical function in a utility .c file. And it doesn't seem this function is used at all so it should be deleted. aq_to_posix should be static const. And if it's really necessary, I think it would be simpler to read using code without the cast and negation. if (aq_rc < 0 || aq_rc >= ARRAY_SIZE(aq_to_posix)) return -ERANGE;
Re: [PATCH net 6/9] MAINTAINERS: mtk-eth: remove Felix
On Mon, 2021-01-11 at 09:41 -0800, Jakub Kicinski wrote: > On Sun, 10 Jan 2021 21:45:46 -0800 Joe Perches wrote: > > On Sun, 2021-01-10 at 21:27 -0800, Jakub Kicinski wrote: > > > Drop Felix from Mediatek Ethernet driver maintainers. > > > We haven't seen any tags since the initial submission. > > [] > > > diff --git a/MAINTAINERS b/MAINTAINERS > > [] > > > @@ -11165,7 +11165,6 @@ F: > > > Documentation/devicetree/bindings/dma/mtk-* > > > F: drivers/dma/mediatek/ > > > > > > > > > MEDIATEK ETHERNET DRIVER > > > -M: Felix Fietkau > > > M: John Crispin > > > M: Sean Wang > > > M: Mark Lee > > > > I think your script is broken as there are multiple subdirectories > > for this entry and > > Quite the opposite, the script intentionally only counts contributions > only to the code under the MAINTAINERS entry. My mistake. I'd seen Felix's name fairly often. I ran this command: $ git log --pretty=oneline --since=5-years-ago --grep="-by: Felix Fietkau" drivers/net/ethernet/mediatek/ 656e705243fd0c2864b89634ea16ed444ef64dc6 net-next: mediatek: add support for MT7623 ethernet Saw that felix had worked on mediatek and then looked up files in drivers/net with mediatek and conflated the wireless files and ethernet files. btw: because I didn't see the script published and so can't verify what's being done here, the MAINTAINERS file shows: F: *Files* and directories wildcard patterns. A trailing slash includes all files and subdirectory files. F: drivers/net/all files in and below drivers/net F: drivers/net/* all files in drivers/net, but not below cheers, Joe
Re: [PATCH net 6/9] MAINTAINERS: mtk-eth: remove Felix
On Sun, 2021-01-10 at 21:27 -0800, Jakub Kicinski wrote: > Drop Felix from Mediatek Ethernet driver maintainers. > We haven't seen any tags since the initial submission. [] > diff --git a/MAINTAINERS b/MAINTAINERS [] > @@ -11165,7 +11165,6 @@ F:Documentation/devicetree/bindings/dma/mtk-* > F: drivers/dma/mediatek/ > > > MEDIATEK ETHERNET DRIVER > -M: Felix Fietkau > M: John Crispin > M: Sean Wang > M: Mark Lee I think your script is broken as there are multiple subdirectories for this entry and Felix is active. For instance: commit 9716ef046b46a6426b2a11301ea5232fc8cdce63 Author: Felix Fietkau Date: Sat Nov 21 18:23:35 2020 +0100 mt76: attempt to free up more room when filling the tx queue Run dma cleanup immediately if the queue is almost full, instead of waiting for the tx interrupt Signed-off-by: Felix Fietkau
Re: [PATCH 05/10] dma: tx49 removal
On Tue, 2021-01-05 at 15:02 +0100, Thomas Bogendoerfer wrote:
> Signed-off-by: Thomas Bogendoerfer
[]
> diff --git a/drivers/dma/txx9dmac.h b/drivers/dma/txx9dmac.h
[]
> @@ -26,11 +26,6 @@
> * DMA channel.
> */
>
>
> -#ifdef CONFIG_MACH_TX49XX
> -static inline bool txx9_dma_have_SMPCHN(void)
> -{
> - return true;
> -}
> #define TXX9_DMA_USE_SIMPLE_CHAIN
> #else
> static inline bool txx9_dma_have_SMPCHN(void)
This doesn't look like it compiles as there's now an #else
without an #if
Re: [PATCH] drivers: net: wireless: realtek: Fix the word association defautly de-faulty
On Wed, 2021-01-06 at 18:48 +0530, Bhaskar Chowdhury wrote: > Good point Randy, there were several driver file witch have "defautly" in it > and I tried to correct that.Only that spell made it a "de-faulty" as dic > suggested . But I think it should be "by default" as you said. What tool suggested 'de-faulty' with a dash between de and faulty" I don't believe it was codespell. $ codespell drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c:61: cacluated ==> calculated drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c:190: cacluated ==> calculated drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c:435: managment ==> management drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c:817: defautly ==> defaultly Nor checkpatch: $ ./scripts/checkpatch.pl -f --terse --nosummary --types=typo_spelling drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c:61: CHECK: 'cacluated' may be misspelled - perhaps 'calculated'? drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c:190: CHECK: 'cacluated' may be misspelled - perhaps 'calculated'? drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c:435: CHECK: 'managment' may be misspelled - perhaps 'management'?
Re: [PATCH] drivers: net: wireless: realtek: Fix the word association defautly de-faulty
On Tue, 2021-01-05 at 17:11 +0530, Bhaskar Chowdhury wrote: > On 22:24 Tue 05 Jan 2021, Julian Calaby wrote: > > Hi Bhaskar, [] > > and your change is just making this comment worse. > really??? Not sure about it. I agree with Julian. I'm fairly sure it's worse. The change you suggest doesn't parse well and is extremely odd. If you _really_ want to just change this use (and the others), I repeat his suggestion of "by default".
Re: [PATCH] liquidio: fix: warning: %u in format string (no. 3) requires 'unsigned int' but the argument type is 'signed int'.
On Wed, 2020-12-30 at 14:41 +0800, YANG LI wrote:
> For safety, modify '%u' to '%d' to keep the type consistent.
There is no additional safety here.
The for loop ensures that i is positive as num_ioq_vector is also
int and so i can not be negative as it's incremented from 0.
> diff --git a/drivers/net/ethernet/cavium/liquidio/lio_core.c
> b/drivers/net/ethernet/cavium/liquidio/lio_core.c
[]
> @@ -1109,12 +1109,12 @@ int octeon_setup_interrupt(struct octeon_device *oct,
> u32 num_ioqs)
> for (i = 0 ; i < num_ioq_vectors ; i++) {
> if (OCTEON_CN23XX_PF(oct))
> snprintf(&queue_irq_names[IRQ_NAME_OFF(i)],
> - INTRNAMSIZ, "LiquidIO%u-pf%u-rxtx-%u",
> + INTRNAMSIZ, "LiquidIO%u-pf%u-rxtx-%d",
> oct->octeon_id, oct->pf_num, i);
>
>
> if (OCTEON_CN23XX_VF(oct))
> snprintf(&queue_irq_names[IRQ_NAME_OFF(i)],
> - INTRNAMSIZ, "LiquidIO%u-vf%u-rxtx-%u",
> + INTRNAMSIZ, "LiquidIO%u-vf%u-rxtx-%d",
> oct->octeon_id, oct->vf_num, i);
>
>
> irqret = request_irq(msix_entries[i].vector,
Re: [PATCH] liquidio: style: Identical condition and return expression 'retval', return value is always 0.
On Wed, 2020-12-30 at 14:07 +0800, YANG LI wrote: > The warning was because of the following line in function > liquidio_set_fec(): > > retval = wait_for_sc_completion_timeout(oct, sc, 0); > if (retval) > return (-EIO); I presume abaci is a robot Perhaps also the robot could look for code immediately above this like: oct->props[lio->ifidx].fec = var; if (resp->fec_setting == SEAPI_CMD_FEC_SET_RS) oct->props[lio->ifidx].fec = 1; else oct->props[lio->ifidx].fec = 0; where a location is immediately overwritten. so the line oct->props[lio->ifidx].fec = var; could be highlighted and perhaps removed and also perhaps the second test and set block could be written oct->props[lio->ifidx].fec = resp->fec_setting == SEAPI_CMD_FEC_SET_RS;
Re: [PATCH net-next] net/mlx5: Use kzalloc for allocating only one thing
On Tue, 2020-12-29 at 21:53 +0800, Zheng Yongjun wrote: > Use kzalloc rather than kcalloc(1,...) [] > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/dr_action.c > b/drivers/net/ethernet/mellanox/mlx5/core/steering/dr_action.c [] > @@ -1782,7 +1782,7 @@ static int dr_action_create_modify_action(struct > mlx5dr_domain *dmn, > if (!chunk) > return -ENOMEM; > > > - hw_actions = kcalloc(1, max_hw_actions * DR_MODIFY_ACTION_SIZE, > GFP_KERNEL); > + hw_actions = kzalloc(max_hw_actions * DR_MODIFY_ACTION_SIZE, > GFP_KERNEL); Perhaps instead: hw_actions = kcalloc(max_hw_actions, DR_MODIFY_ACTION_SIZE, GFP_KERNEL);
Re: [PATCH] ethernet: Remove invalid trailers after %pI4
On Sat, 2020-12-26 at 09:10 -0800, Joe Perches wrote: > Alphanumeric characters after vsprintf pointer extension %pI4 are > not valid and are not emitted. > > Remove the invalid characters from the %pI4 uses. self-nak. I believe I misunderstood the format specifier.
[PATCH] ethernet: Remove invalid trailers after %pI4
Alphanumeric characters after vsprintf pointer extension %pI4 are
not valid and are not emitted.
Remove the invalid characters from the %pI4 uses.
Signed-off-by: Joe Perches
---
drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 6 +++---
drivers/net/ethernet/intel/i40e/i40e_main.c | 4 ++--
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 2 +-
drivers/net/ethernet/qlogic/qed/qed_iwarp.c | 4 ++--
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
index 5e4429b14b8c..213cbdea3888 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
@@ -1232,7 +1232,7 @@ static int bnxt_tc_resolve_tunnel_hdrs(struct bnxt *bp,
rt = ip_route_output_key(dev_net(real_dst_dev), &flow);
if (IS_ERR(rt)) {
- netdev_info(bp->dev, "no route to %pI4b\n", &flow.daddr);
+ netdev_info(bp->dev, "no route to %pI4\n", &flow.daddr);
return -EOPNOTSUPP;
}
@@ -1258,7 +1258,7 @@ static int bnxt_tc_resolve_tunnel_hdrs(struct bnxt *bp,
#endif
} else if (dst_dev != real_dst_dev) {
netdev_info(bp->dev,
- "dst_dev(%s) for %pI4b is not PF-if(%s)\n",
+ "dst_dev(%s) for %pI4 is not PF-if(%s)\n",
netdev_name(dst_dev), &flow.daddr,
netdev_name(real_dst_dev));
rc = -EOPNOTSUPP;
@@ -1267,7 +1267,7 @@ static int bnxt_tc_resolve_tunnel_hdrs(struct bnxt *bp,
nbr = dst_neigh_lookup(&rt->dst, &flow.daddr);
if (!nbr) {
- netdev_info(bp->dev, "can't lookup neighbor for %pI4b\n",
+ netdev_info(bp->dev, "can't lookup neighbor for %pI4\n",
&flow.daddr);
rc = -EOPNOTSUPP;
goto put_rt;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 1db482d310c2..eab6ce63b63d 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -7924,7 +7924,7 @@ static int i40e_parse_cls_flower(struct i40e_vsi *vsi,
if (match.mask->dst == cpu_to_be32(0x)) {
field_flags |= I40E_CLOUD_FIELD_IIP;
} else {
- dev_err(&pf->pdev->dev, "Bad ip dst mask
%pI4b\n",
+ dev_err(&pf->pdev->dev, "Bad ip dst mask
%pI4\n",
&match.mask->dst);
return I40E_ERR_CONFIG;
}
@@ -7934,7 +7934,7 @@ static int i40e_parse_cls_flower(struct i40e_vsi *vsi,
if (match.mask->src == cpu_to_be32(0x)) {
field_flags |= I40E_CLOUD_FIELD_IIP;
} else {
- dev_err(&pf->pdev->dev, "Bad ip src mask
%pI4b\n",
+ dev_err(&pf->pdev->dev, "Bad ip src mask
%pI4\n",
&match.mask->src);
return I40E_ERR_CONFIG;
}
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index 41424ee909a0..6c711385aae9 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -2297,7 +2297,7 @@ static void mlxsw_sp_router_neigh_ent_ipv4_process(struct
mlxsw_sp *mlxsw_sp,
if (!n)
return;
- netdev_dbg(dev, "Updating neighbour with IP=%pI4h\n", &dip);
+ netdev_dbg(dev, "Updating neighbour with IP=%pI4\n", &dip);
neigh_event_send(n, NULL);
neigh_release(n);
}
diff --git a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
index a99861124630..6756f7919deb 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
@@ -584,7 +584,7 @@ qed_iwarp_print_tcp_ramrod(struct qed_hwfn *p_hwfn,
if (p_tcp_ramrod->tcp.ip_version == TCP_IPV4) {
DP_VERBOSE(p_hwfn, QED_MSG_RDMA,
- "local_ip=%pI4h:%x, remote_ip=%pI4h:%x, vlan=%x\n",
+ "local_ip=%pI4:%x, remote_ip=%pI4:%x, vlan=%x\n",
p_tcp_ramrod->tcp.local_ip,
p_tcp_ramrod->tcp.local_port,
p_
Re: [PATCH] nfp: remove h from printk format specifier
On Fri, 2020-12-25 at 14:13 -0800, Tom Rix wrote: > On 12/25/20 9:06 AM, Joe Perches wrote: > > On Fri, 2020-12-25 at 06:56 -0800, Tom Rix wrote: > > > On 12/24/20 2:39 PM, Joe Perches wrote: > > [] > > > > Kernel code doesn't use a signed char or short with %hx or %hu very > > > > often > > > > but in case you didn't already know, any signed char/short emitted with > > > > anything like %hx or %hu needs to be left alone as sign extension > > > > occurs so: > > > Yes, this would also effect checkpatch. > > Of course but checkpatch is stupid and doesn't know types > > so it just assumes that the type argument is not signed. > > > > In general, that's a reasonable but imperfect assumption. > > > > coccinelle could probably do this properly as it's a much > > better parser. clang-tidy should be able to as well. > > > Ok. > > But types not matching the format string is a larger problem. > > Has there been an effort to clean these up ? Not really no. __printf already does a reasonable job for that. The biggest issue for format type mismatches is the %p extensions. __printf can only verify that the argument is a pointer, not necessarily the 'right' type of pointed to object. There are overflow possibilities like '"%*ph", len, pointer' where pointer may not have len bytes available and, for instance, mismatched uses of %pI4 and %pI6 where %pI4 expects a pointer to 4 bytes and %pI6 expects a pointer to 16 bytes. Anyway it's not that easy a problem to analyze.
Re: [PATCH] nfp: remove h from printk format specifier
On Fri, 2020-12-25 at 06:56 -0800, Tom Rix wrote: > On 12/24/20 2:39 PM, Joe Perches wrote: [] > > Kernel code doesn't use a signed char or short with %hx or %hu very often > > but in case you didn't already know, any signed char/short emitted with > > anything like %hx or %hu needs to be left alone as sign extension occurs so: > > Yes, this would also effect checkpatch. Of course but checkpatch is stupid and doesn't know types so it just assumes that the type argument is not signed. In general, that's a reasonable but imperfect assumption. coccinelle could probably do this properly as it's a much better parser. clang-tidy should be able to as well.
Re: [PATCH] nfp: remove h from printk format specifier
On Thu, 2020-12-24 at 14:14 -0800, Tom Rix wrote:
> On 12/24/20 12:21 PM, Simon Horman wrote:
> > On Wed, Dec 23, 2020 at 12:20:53PM -0800, t...@redhat.com wrote:
> > > From: Tom Rix
> > >
> > > This change fixes the checkpatch warning described in this commit
> > > commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of
> > > unnecessary %h[xudi] and %hh[xudi]")
> > >
> > > Standard integer promotion is already done and %hx and %hhx is useless
> > > so do not encourage the use of %hh[xudi] or %h[xudi].
> > >
> > > Signed-off-by: Tom Rix
> > Hi Tom,
> >
> > This patch looks appropriate for net-next, which is currently closed.
> >
> > The changes look fine, but I'm curious to know if its intentionally that
> > the following was left alone in
> > ethernet/netronome/nfp/nfp_net_ethtool.c:nfp_net_get_nspinfo()
> >
> > snprintf(version, ETHTOOL_FWVERS_LEN, "%hu.%hu"
>
> I am limiting changes to logging functions, what is roughly in checkpatch.
>
> I can add this snprintf in if you want.
I'm a bit confused here Tom.
I thought your clang-tidy script was looking for anything marked with
__printf() that is using %h[idux] or %hh[idux].
Wouldn't snprintf qualify for this already?
include/linux/kernel.h-extern __printf(3, 4)
include/linux/kernel.h:int snprintf(char *buf, size_t size, const char *fmt,
...);
Kernel code doesn't use a signed char or short with %hx or %hu very often
but in case you didn't already know, any signed char/short emitted with
anything like %hx or %hu needs to be left alone as sign extension occurs so:
signed char foo = -1;
printk("%hx", foo);
emits but
printk("%x", foo);
emits
An example:
$ gcc -x c -
#include
#include
int main(int argc, char **argv)
{
signed short i = -1;
printf("hx: %hx\n", i);
printf("x: %x\n", i);
printf("hu: %hu\n", i);
printf("u: %u\n", i);
return 0;
}
$ ./a.out
hx:
x:
hu: 65535
u: 4294967295
$
Re: [PATCH] amd-xgbe: remove h from printk format specifier
On Wed, 2020-12-23 at 12:33 -0800, Tom Rix wrote:
> On 12/23/20 12:14 PM, Joe Perches wrote:
> > On Wed, 2020-12-23 at 11:43 -0800, t...@redhat.com wrote:
> > > From: Tom Rix
> > >
> > > This change fixes the checkpatch warning described in this commit
> > > commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of
> > > unnecessary %h[xudi] and %hh[xudi]")
> > >
> > > Standard integer promotion is already done and %hx and %hhx is useless
> > > so do not encourage the use of %hh[xudi] or %h[xudi].
> > Why only xgbe-ethtool?
> >
> > Perhaps your script only converts direct uses of functions
> > marked with __printf and not any uses of the same functions
> > via macros.
>
> The fixer may have issues.
Perhaps until the fixer is fixed, it'd be more
complete coverage to use checkpatch like:
$ git ls-files | \
xargs ./scripts/checkpatch.pl -f --fix-inplace --types=unnecessary_modifier
e.g.:
$ git ls-files drivers/net/ethernet/amd/xgbe | \
xargs ./scripts/checkpatch.pl -f --fix-inplace --types=unnecessary_modifier
$ git diff -U0 drivers/net/ethernet/amd/xgbe/
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-dcb.c
b/drivers/net/ethernet/amd/xgbe/xgbe-dcb.c
index 895d35639129..dcd2a181d43a 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-dcb.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-dcb.c
@@ -155 +155 @@ static int xgbe_dcb_ieee_setets(struct net_device *netdev,
- "TC%u: tx_bw=%hhu, rx_bw=%hhu, tsa=%hhu\n", i,
+ "TC%u: tx_bw=%u, rx_bw=%u, tsa=%u\n", i,
@@ -158 +158 @@ static int xgbe_dcb_ieee_setets(struct net_device *netdev,
- netif_dbg(pdata, drv, netdev, "PRIO%u: TC=%hhu\n", i,
+ netif_dbg(pdata, drv, netdev, "PRIO%u: TC=%u\n", i,
@@ -233 +233 @@ static int xgbe_dcb_ieee_setpfc(struct net_device *netdev,
- "cap=%hhu, en=%#hhx, mbc=%hhu, delay=%hhu\n",
+ "cap=%u, en=%#x, mbc=%u, delay=%u\n",
@@ -267 +267 @@ static u8 xgbe_dcb_setdcbx(struct net_device *netdev, u8 dcbx)
- netif_dbg(pdata, drv, netdev, "DCBX=%#hhx\n", dcbx);
+ netif_dbg(pdata, drv, netdev, "DCBX=%#x\n", dcbx);
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
index d5fd49dd25f3..ff0cd94bb91a 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
@@ -488 +488 @@ static void xgbe_set_vxlan_id(struct xgbe_prv_data *pdata)
- netif_dbg(pdata, drv, pdata->netdev, "VXLAN tunnel id set to %hx\n",
+ netif_dbg(pdata, drv, pdata->netdev, "VXLAN tunnel id set to %x\n",
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
index 2709a2db5657..0ae16bc87833 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
@@ -2781 +2781 @@ void xgbe_print_pkt(struct net_device *netdev, struct sk_buff
*skb, bool tx_rx)
- netdev_dbg(netdev, "Protocol: %#06hx\n", ntohs(eth->h_proto));
+ netdev_dbg(netdev, "Protocol: %#06x\n", ntohs(eth->h_proto));
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
b/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
index 61f39a0e04f9..3c18f26bf2a5 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
@@ -342 +342 @@ static int xgbe_set_link_ksettings(struct net_device *netdev,
- netdev_err(netdev, "invalid phy address %hhu\n",
+ netdev_err(netdev, "invalid phy address %u\n",
@@ -349 +349 @@ static int xgbe_set_link_ksettings(struct net_device *netdev,
- netdev_err(netdev, "unsupported autoneg %hhu\n",
+ netdev_err(netdev, "unsupported autoneg %u\n",
@@ -361 +361 @@ static int xgbe_set_link_ksettings(struct net_device *netdev,
- netdev_err(netdev, "unsupported duplex %hhu\n",
+ netdev_err(netdev, "unsupported duplex %u\n",
Re: [PATCH] amd-xgbe: remove h from printk format specifier
On Wed, 2020-12-23 at 11:43 -0800, t...@redhat.com wrote:
> From: Tom Rix
>
> This change fixes the checkpatch warning described in this commit
> commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of
> unnecessary %h[xudi] and %hh[xudi]")
>
> Standard integer promotion is already done and %hx and %hhx is useless
> so do not encourage the use of %hh[xudi] or %h[xudi].
Why only xgbe-ethtool?
Perhaps your script only converts direct uses of functions
marked with __printf and not any uses of the same functions
via macros.
$ git grep -P '%[\w\d\.]*h\w' drivers/net/ethernet/amd/xgbe/
drivers/net/ethernet/amd/xgbe/xgbe-dcb.c: "TC%u:
tx_bw=%hhu, rx_bw=%hhu, tsa=%hhu\n", i,
drivers/net/ethernet/amd/xgbe/xgbe-dcb.c: netif_dbg(pdata, drv,
netdev, "PRIO%u: TC=%hhu\n", i,
drivers/net/ethernet/amd/xgbe/xgbe-dcb.c:
"unsupported TSA algorithm (%hhu)\n",
drivers/net/ethernet/amd/xgbe/xgbe-dcb.c: "cap=%hhu, en=%#hhx,
mbc=%hhu, delay=%hhu\n",
drivers/net/ethernet/amd/xgbe/xgbe-dev.c: netif_dbg(pdata, drv,
pdata->netdev, "VXLAN tunnel id set to %hx\n",
drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c: netdev_err(netdev,
"invalid phy address %hhu\n",
drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c: netdev_err(netdev,
"unsupported autoneg %hhu\n",
drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c:
netdev_err(netdev, "unsupported duplex %hhu\n",
>
> Signed-off-by: Tom Rix
> ---
> drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
> b/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
> index 61f39a0e04f9..3c18f26bf2a5 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
> @@ -339,14 +339,14 @@ static int xgbe_set_link_ksettings(struct net_device
> *netdev,
> speed = cmd->base.speed;
>
>
> if (cmd->base.phy_address != pdata->phy.address) {
> - netdev_err(netdev, "invalid phy address %hhu\n",
> + netdev_err(netdev, "invalid phy address %u\n",
> cmd->base.phy_address);
> return -EINVAL;
> }
>
>
> if ((cmd->base.autoneg != AUTONEG_ENABLE) &&
> (cmd->base.autoneg != AUTONEG_DISABLE)) {
> - netdev_err(netdev, "unsupported autoneg %hhu\n",
> + netdev_err(netdev, "unsupported autoneg %u\n",
> cmd->base.autoneg);
> return -EINVAL;
> }
> @@ -358,7 +358,7 @@ static int xgbe_set_link_ksettings(struct net_device
> *netdev,
> }
>
>
> if (cmd->base.duplex != DUPLEX_FULL) {
> - netdev_err(netdev, "unsupported duplex %hhu\n",
> + netdev_err(netdev, "unsupported duplex %u\n",
> cmd->base.duplex);
> return -EINVAL;
> }
Re: [PATCH net] MAINTAINERS: remove names from mailing list maintainers
On Wed, 2020-12-23 at 02:50 +, patchwork-bot+netdev...@kernel.org wrote: > Hello: > > This patch was applied to netdev/net.git (refs/heads/master): > > On Sat, 19 Dec 2020 10:55:38 -0800 you wrote: > > When searching for inactive maintainers it's useful to filter > > out mailing list addresses. Such "maintainers" will obviously > > never feature in a "From:" line of an email or a review tag. > > > > Since "L:" entries only provide the address of a mailing list > > without a fancy name extend this pattern to "M:" entries. > > > > [...] > > Here is the summary with links: > - [net] MAINTAINERS: remove names from mailing list maintainers > https://git.kernel.org/netdev/net/c/8b0f64b113d6 > > You are awesome, thank you! I still think this is not a good patch nor mechanism to show what is generally used as exploders rather than individuals. Effectively only individuals can submit patches and so can be M: Maintainers. I believe these entries should really use R: Reviewer entries and keep the descriptive naming content. The descriptive naming does add value and this patch loses that value.
Re: [PATCH net] MAINTAINERS: remove names from mailing list maintainers
On 2020-12-19 10:55, Jakub Kicinski wrote: When searching for inactive maintainers it's useful to filter out mailing list addresses. Such "maintainers" will obviously never feature in a "From:" line of an email or a review tag. Since "L:" entries only provide the address of a mailing list without a fancy name extend this pattern to "M:" entries. As these are not actual people I suggest using R: entries and not removing the more descriptive names.
Re: [PATCH] atm: horizon: remove h from printk format specifier
On Tue, 2020-12-15 at 06:24 -0800, t...@redhat.com wrote:
> From: Tom Rix
>
> See Documentation/core-api/printk-formats.rst.
> h should no longer be used in the format specifier for printk.
>
> Signed-off-by: Tom Rix
> ---
> drivers/atm/horizon.c | 6 +++---
Chas?
Madge has been out of business for a couple decades now.
I doubt anyone does but does anyone actually use this driver or
even have working hardware?
If not, how about just deleting this driver altogether instead?
from horizon.h:
/*
Madge Horizon ATM Adapter driver.
Copyright (C) 1995-1999 Madge Networks Ltd.
*/
/*
IMPORTANT NOTE: Madge Networks no longer makes the adapters
supported by this driver and makes no commitment to maintain it.
*/
> diff --git a/drivers/atm/horizon.c b/drivers/atm/horizon.c
[]
> @@ -1609,7 +1609,7 @@ static int hrz_send (struct atm_vcc * atm_vcc, struct
> sk_buff * skb) {
> if (*s++ == 'D') {
> for (i = 0; i < 4; ++i)
> d = (d << 4) | hex_to_bin(*s++);
> - PRINTK (KERN_INFO, "debug bitmap is now %hx", debug = d);
> + PRINTK (KERN_INFO, "debug bitmap is now %x", debug = d);
An IMO ugly assignment in a printk too.
Re: [PATCH v2] net/mlx4: Use true,false for bool variable
On Tue, 2020-12-15 at 07:18 +0200, Leon Romanovsky wrote: > On Mon, Dec 14, 2020 at 11:15:01AM -0800, Joe Perches wrote: > > I prefer revisions to single patches (as opposed to large patch series) > > in the same thread. > > It depends which side you are in that game. From the reviewer point of > view, such submission breaks flow very badly. It unfolds the already > reviewed thread, messes with the order and many more little annoying > things. This is where I disagree with you. I am a reviewer here. Not having context to be able to inspect vN -> vN+1 is made more difficult not having the original patch available and having to search history for it. Almost no one adds URL links to older submissions below the ---. Were that a standard mechanism below the --- line, then it would be OK.
Re: [PATCH v2] net/mlx4: Use true,false for bool variable
On Mon, 2020-12-14 at 11:03 -0800, Jakub Kicinski wrote: > On Mon, 14 Dec 2020 13:16:08 +0200 Leon Romanovsky wrote: > > On Mon, Dec 14, 2020 at 11:30:08AM +0100, Vasyl Gomonovych wrote: > > > It is fix for semantic patch warning available in > > > scripts/coccinelle/misc/boolinit.cocci > > > Fix en_rx.c:687:1-17: WARNING: Assignment of 0/1 to bool variable > > > Fix main.c:4465:5-13: WARNING: Comparison of 0/1 to bool variable > > > > > > Signed-off-by: Vasyl Gomonovych > > > --- > > > - Add coccicheck script name > > > - Simplify if condition > > > --- > > > drivers/net/ethernet/mellanox/mlx4/en_rx.c | 2 +- > > > drivers/net/ethernet/mellanox/mlx4/main.c | 2 +- > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > Please refrain from sending new version of patches as reply-to to > > previous variants. It makes to appear previous patches out-of-order > > while viewing in threaded mode. > > Yes, please! I'm glad I'm not the only one who feels this way! :) I'm the other way. I prefer revisions to single patches (as opposed to large patch series) in the same thread. There is no other easy way for changes to a patch to be tracked AFAIK. Most email clients use both In-Reply-To: and References: headers as the mechanism to thread replies. Keeping the latest messages at the bottom of a thread works well to see revision sequences.
Re: [PATCH] netfilter: conntrack: fix -Wformat
On Sun, 2020-12-13 at 11:21 -0800, Tom Rix wrote:
> On 12/2/20 2:34 PM, Nick Desaulniers wrote:
> > On Tue, Nov 10, 2020 at 2:04 PM Joe Perches wrote:
> > > On Tue, 2020-11-10 at 14:00 -0800, Nick Desaulniers wrote:
> > >
> > > > Yeah, we could go through and remove %h and %hh to solve this, too,
> > > > right?
> > > Yup.
> > >
> > > I think one of the checkpatch improvement mentees is adding
> > > some suggestion and I hope an automated fix mechanism for that.
> > >
> > > https://lore.kernel.org/lkml/5e3265c241602bb54286fbaae9222070daa4768e.ca...@perches.com/
> > + Tom, who's been looking at leveraging clang-tidy to automate such
> > treewide mechanical changes.
> > ex. https://reviews.llvm.org/D91789
> >
> > See also commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging
> > use of unnecessary %h[xudi] and %hh[xudi]") for a concise summary of
> > related context.
>
> I have posted the fixer here
>
> https://reviews.llvm.org/D93182
>
> It catches about 200 problems in 100 files, I'll be posting these soon.
Thanks, but see below:
> clang-tidy-fix's big difference over checkpatch is using the __printf(x,y)
> attribute to find the log functions.
>
> I will be doing a follow-on to add the missing __printf or __scanf's and
> rerunning the fixer.
scanf should not be tested because the %h use is required there.
Re: [PATCH] net/mlx4: Use true,false for bool variable
On Fri, 2020-12-11 at 11:05 +0100, Vasyl Gomonovych wrote:
> Fix en_rx.c:687:1-17: WARNING: Assignment of 0/1 to bool variable
> Fix main.c:4465:5-13: WARNING: Comparison of 0/1 to bool variable
[]
> diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c
> b/drivers/net/ethernet/mellanox/mlx4/main.c
[]
> @@ -4462,7 +4462,7 @@ static int __init mlx4_verify_params(void)
> pr_warn("mlx4_core: log_num_vlan - obsolete module param, using
> %d\n",
> MLX4_LOG_NUM_VLANS);
>
>
> - if (use_prio != 0)
> + if (use_prio != false)
> pr_warn("mlx4_core: use_prio - obsolete module param,
> ignored\n");
Generally, assuming use_prio is bool, this would be written
if (use_prio)
pr_warn("etc...")
Re: [PATCH wireless -next] cw1200: txrx: convert comma to semicolon
On Thu, 2020-12-10 at 18:50 +, Kalle Valo wrote:
> Zheng Yongjun wrote:
>
> > Replace a comma between expression statements by a semicolon.
> >
> > Signed-off-by: Zheng Yongjun
>
> Patch applied to wireless-drivers-next.git, thanks.
>
> c42d492c672a cw1200: txrx: convert comma to semicolon
>
There's are several of these in drivers/net/wireless:
Using a cocci script posted by Julia:
---
diff -u -p a/mediatek/mt76/mt7915/mcu.c b/mediatek/mt76/mt7915/mcu.c
--- a/mediatek/mt76/mt7915/mcu.c
+++ b/mediatek/mt76/mt7915/mcu.c
@@ -1148,7 +1148,7 @@ mt7915_mcu_sta_ba_tlv(struct sk_buff *sk
tlv = mt7915_mcu_add_tlv(skb, STA_REC_BA, sizeof(*ba));
ba = (struct sta_rec_ba *)tlv;
- ba->ba_type = tx ? MT_BA_TYPE_ORIGINATOR : MT_BA_TYPE_RECIPIENT,
+ ba->ba_type = tx ? MT_BA_TYPE_ORIGINATOR : MT_BA_TYPE_RECIPIENT;
ba->winsize = cpu_to_le16(params->buf_size);
ba->ssn = cpu_to_le16(params->ssn);
ba->ba_en = enable << params->tid;
@@ -1676,7 +1676,7 @@ mt7915_mcu_wtbl_ht_tlv(struct sk_buff *s
tlv = mt7915_mcu_add_nested_tlv(skb, WTBL_VHT, sizeof(*vht),
wtbl_tlv, sta_wtbl);
vht = (struct wtbl_vht *)tlv;
- vht->ldpc = sta->vht_cap.cap & IEEE80211_VHT_CAP_RXLDPC,
+ vht->ldpc = sta->vht_cap.cap & IEEE80211_VHT_CAP_RXLDPC;
vht->vht = true;
af =
FIELD_GET(IEEE80211_VHT_CAP_MAX_A_MPDU_LENGTH_EXPONENT_MASK,
@@ -2858,7 +2858,7 @@ int mt7915_mcu_init(struct mt7915_dev *d
};
int ret;
- dev->mt76.mcu_ops = &mt7915_mcu_ops,
+ dev->mt76.mcu_ops = &mt7915_mcu_ops;
ret = mt7915_driver_own(dev);
if (ret)
diff -u -p a/mediatek/mt76/mt7615/usb_mcu.c b/mediatek/mt76/mt7615/usb_mcu.c
--- a/mediatek/mt76/mt7615/usb_mcu.c
+++ b/mediatek/mt76/mt7615/usb_mcu.c
@@ -61,7 +61,7 @@ int mt7663u_mcu_init(struct mt7615_dev *
};
int ret;
- dev->mt76.mcu_ops = &mt7663u_mcu_ops,
+ dev->mt76.mcu_ops = &mt7663u_mcu_ops;
/* usb does not support runtime-pm */
clear_bit(MT76_STATE_PM, &dev->mphy.state);
diff -u -p a/mediatek/mt76/mt7615/mcu.c b/mediatek/mt76/mt7615/mcu.c
--- a/mediatek/mt76/mt7615/mcu.c
+++ b/mediatek/mt76/mt7615/mcu.c
@@ -982,7 +982,7 @@ mt7615_mcu_sta_ba_tlv(struct sk_buff *sk
tlv = mt7615_mcu_add_tlv(skb, STA_REC_BA, sizeof(*ba));
ba = (struct sta_rec_ba *)tlv;
- ba->ba_type = tx ? MT_BA_TYPE_ORIGINATOR : MT_BA_TYPE_RECIPIENT,
+ ba->ba_type = tx ? MT_BA_TYPE_ORIGINATOR : MT_BA_TYPE_RECIPIENT;
ba->winsize = cpu_to_le16(params->buf_size);
ba->ssn = cpu_to_le16(params->ssn);
ba->ba_en = enable << params->tid;
@@ -2472,7 +2472,7 @@ int mt7615_mcu_init(struct mt7615_dev *d
};
int ret;
- dev->mt76.mcu_ops = &mt7615_mcu_ops,
+ dev->mt76.mcu_ops = &mt7615_mcu_ops;
ret = mt7615_mcu_drv_pmctrl(dev);
if (ret)
diff -u -p a/mediatek/mt76/mt7615/sdio_mcu.c b/mediatek/mt76/mt7615/sdio_mcu.c
--- a/mediatek/mt76/mt7615/sdio_mcu.c
+++ b/mediatek/mt76/mt7615/sdio_mcu.c
@@ -139,7 +139,7 @@ int mt7663s_mcu_init(struct mt7615_dev *
if (ret)
return ret;
- dev->mt76.mcu_ops = &mt7663s_mcu_ops,
+ dev->mt76.mcu_ops = &mt7663s_mcu_ops;
ret = mt76_get_field(dev, MT_CONN_ON_MISC, MT_TOP_MISC2_FW_N9_RDY);
if (ret) {
diff -u -p a/st/cw1200/txrx.c b/st/cw1200/txrx.c
--- a/st/cw1200/txrx.c
+++ b/st/cw1200/txrx.c
@@ -650,7 +650,7 @@ cw1200_tx_h_rate_policy(struct cw1200_co
wsm->flags |= t->txpriv.rate_id << 4;
t->rate = cw1200_get_tx_rate(priv,
- &t->tx_info->control.rates[0]),
+ &t->tx_info->control.rates[0]);
wsm->max_tx_rate = t->rate->hw_value;
if (t->rate->flags & IEEE80211_TX_RC_MCS) {
if (cw1200_ht_greenfield(&priv->ht_info))
diff -u -p a/intersil/prism54/islpci_dev.c b/intersil/prism54/islpci_dev.c
--- a/intersil/prism54/islpci_dev.c
+++ b/intersil/prism54/islpci_dev.c
@@ -121,7 +121,7 @@ isl_upload_firmware(islpci_private *priv
while (_fw_len > 0) {
/* use non-swapping writel() */
__raw_writel(*fw_ptr, dev_fw_ptr);
- fw_ptr++, dev_fw_ptr++;
+ fw_ptr++; dev_fw_ptr++;
_fw_len -= 4;
}
diff -u -p a/intel/iwlwifi/mvm/debugfs.c b/intel/iwlwifi/mvm/debugfs.c
--- a/intel/iwlwifi/mvm/debugfs.c
+++ b/intel/iwlwifi/mvm/debugfs.c
@@ -1949,7 +1949,7 @@ static ssize_t iwl_dbgfs_mem_write(struc
return -EFAULT;
}
- hcmd.flags = CMD_WANT_SKB | CMD_SEND_IN_RFKILL,
+ hcmd.flags = CMD_WANT_SKB | CMD_SEND_IN_RFKILL;
hcmd.data[0] = (void *)cmd;
hcmd.len[0] = cmd_size;
diff -u -p a/ralink/rt2x00/rt2800lib
Re: [PATCH 1/7] net: 8021q: remove unneeded MODULE_VERSION() usage
On Fri, 2020-12-04 at 16:09 -0800, Jakub Kicinski wrote: > On Wed, 2 Dec 2020 13:49:53 +0100 Enrico Weigelt, metux IT consult > wrote: > > Remove MODULE_VERSION(), as it isn't needed at all: the only version > > making sense is the kernel version. > > > > Signed-off-by: Enrico Weigelt, metux IT consult > > Thanks for the patches. Please drop the "metux IT consult" from the > addresses. The from space is supposed to be for your name. If you _really_ want this superfluous 'metux IT consult' content in your signature, and I don't think you should, use parentheses around it. Enrico Weigelt (metux IT consult) Using a comma makes copy/paste into an email client think it's two addresses.
Re: [PATCH AUTOSEL 5.9 22/33] vhost scsi: add lun parser helper
On Fri, 2020-12-04 at 10:49 -0500, Sasha Levin wrote: > On Fri, Dec 04, 2020 at 09:27:28AM +0100, Paolo Bonzini wrote: > > On 01/12/20 00:59, Sasha Levin wrote: > > > > > > It's quite easy to NAK a patch too, just reply saying "no" and it'll be > > > dropped (just like this patch was dropped right after your first reply) > > > so the burden on maintainers is minimal. > > > > The maintainers are _already_ marking patches with "Cc: stable". That > > They're not, though. Some forget, some subsystems don't mark anything, > some don't mark it as it's not stable material when it lands in their > tree but then it turns out to be one if it sits there for too long. > > > (plus backports) is where the burden on maintainers should start and > > end. I don't see the need to second guess them. > > This is similar to describing our CI infrastructure as "second > guessing": why are we second guessing authors and maintainers who are > obviously doing the right thing by testing their patches and reporting > issues to them? > > Are you saying that you have always gotten stable tags right? never > missed a stable tag where one should go? I think this simply adds to the burden of being a maintainer without all that much value. I think the primary value here would be getting people to upgrade to current versions rather than backporting to nominally stable and relatively actively changed old versions. This is very much related to this thread about trivial patches and maintainer burdening: https://lore.kernel.org/lkml/1c7d7fde126bc0acf825766de64bf2f9b888f216.ca...@hansenpartnership.com/
Re: [PATCH] [v9] wireless: Initial driver submission for pureLiFi STA devices
On Thu, 2020-12-03 at 10:39 +0530, Srinivasan Raju wrote:
> This introduces the pureLiFi LiFi driver for LiFi-X, LiFi-XC
> and LiFi-XL USB devices.
The below is a trivial readability possible enhancement and should not
hinder this patch being applied.
> diff --git a/drivers/net/wireless/purelifi/dbgfs.c
> b/drivers/net/wireless/purelifi/dbgfs.c
[]
> +ssize_t purelifi_store_frequency(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t len)
> +{
> + int i, r, row, col, predivider, feedback_divider, output_div_0;
> + int output_div_1, output_div_2, clkout3_phase;
> + char values[6][4] = {{0}, {0} };
> + char usr_input[8] = {0};
> + bool valid = false;
> + int rv = 0;
> +
> + row = 0; col = 0;
> + for (i = 0; i < min(len, sizeof(values)); i++) {
> + values[row][col] = buf[i];
> + col++;
> + if (buf[i] == 0x20) {
> + row++;
> + col = 0;
> + } else if (buf[i] == 0x0A) {
> + if (row == 5 && col > 0) {
> + rv = sscanf((char *)&values[0][0],
> + "%d", &predivider);
Aren't the casts to (char *) redundant ?
> + if (rv != 1)
> + valid = false;
> + rv = sscanf((char *)&values[1][0],
> + "%d", &feedback_divider);
> + if (rv != 1)
> + valid = false;
> + rv = sscanf((char *)&values[2][0],
> + "%d", &output_div_0);
> + if (rv != 1)
> + valid = false;
> + rv = sscanf((char *)&values[3][0],
> + "%d", &output_div_1);
> + if (rv != 1)
> + valid = false;
> + rv = sscanf((char *)&values[4][0],
> + "%d", &output_div_2);
> + if (rv != 1)
> + valid = false;
> + rv = sscanf((char *)&values[5][0],
> + "%d", &clkout3_phase);
> + if (rv != 1)
> + valid = false;
> + valid = true;
trivia: this might be more intelligible with an array and a loop
int *index[] = {
&predivider,
&feedback_divider,
&output_div_0,
&output_div_1,
&output_div_2,
&clkout3_phase
};
valid = true;
i = 0;
while (valid && i < ARRAY_SIZE(index)) {
if (sscanf(&values[i][0], "%d", index[i]) != 1)
valid = false;
i++;
}
> +
> + break;
> + }
> + }
> + }
> +
> + if (valid) {
> + usr_input[0] = (char)predivider;
> + usr_input[1] = (char)feedback_divider;
> + usr_input[2] = (char)output_div_0;
> + usr_input[3] = (char)output_div_1;
> + usr_input[4] = (char)output_div_2;
> + usr_input[5] = (char)clkout3_phase;
> +
> + dev_info(dev, "options IP_DIV: %d VCO_MULT: %d OP_DIV_PHY: %d",
> + usr_input[0], usr_input[1], usr_input[2]);
> + dev_info(dev, "OP_DIV_CPU: %d OP_DIV_USB: %d CLK3_PH: %d\n",
> + usr_input[3], usr_input[4], usr_input[5]);
> +
> + r = usb_write_req((const u8 *)&usr_input[0],
> + sizeof(usr_input), USB_REQ_SET_FREQ);
> + if (r < 0)
> + dev_err(dev, "SET_FREQ failed (%d)\n", r);
> + } else {
> + dev_err(dev, "Your options are invalid\n");
> + }
> +
> + return len;
> +}
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Tue, 2020-11-24 at 11:58 +1100, Finn Thain wrote: > it's not for me to prove that such patches don't affect code > generation. That's for the patch author and (unfortunately) for reviewers. Ideally, that proof would be provided by the compilation system itself and not patch authors nor reviewers nor maintainers. Unfortunately gcc does not guarantee repeatability or deterministic output. To my knowledge, neither does clang.
Re: [PATCH net-next 15/17] rxrpc: Organise connection security to use a union
On Mon, 2020-11-23 at 20:11 +, David Howells wrote:
> Organise the security information in the rxrpc_connection struct to use a
> union to allow for different data for different security classes.
Is there a known future purpose to this?
> diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
> @@ -448,9 +448,15 @@ struct rxrpc_connection {
> struct list_headproc_link; /* link in procfs list */
> struct list_headlink; /* link in master connection
> list */
> struct sk_buff_head rx_queue; /* received conn-level packets
> */
> +
> const struct rxrpc_security *security; /* applied security module */
> - struct crypto_sync_skcipher *cipher;/* encryption handle */
> - struct rxrpc_crypt csum_iv;/* packet checksum base */
> + union {
> + struct {
> + struct crypto_sync_skcipher *cipher;/* encryption
> handle */
> + struct rxrpc_crypt csum_iv; /* packet checksum base
> */
> + u32 nonce; /* response re-use preventer */
> + } rxkad;
> + };
It seems no other follow-on patch in the series uses this nameless union.
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, 2020-11-23 at 07:58 -0800, James Bottomley wrote: > We're also complaining about the inability to recruit maintainers: > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > And burn out: > > http://antirez.com/news/129 https://www.wired.com/story/open-source-coders-few-tired/ > What I'm actually trying to articulate is a way of measuring value of > the patch vs cost ... it has nothing really to do with who foots the > actual bill. It's unclear how to measure value in consistency. But one way that costs can be reduced is by automation and _not_ involving maintainers when the patch itself is provably correct. > One thesis I'm actually starting to formulate is that this continual > devaluing of maintainers is why we have so much difficulty keeping and > recruiting them. The linux kernel has something like 1500 different maintainers listed in the MAINTAINERS file. That's not a trivial number. $ git grep '^M:' MAINTAINERS | sort | uniq -c | wc -l 1543 $ git grep '^M:' MAINTAINERS| cut -f1 -d'<' | sort | uniq -c | wc -l 1446 I think the question you are asking is about trust and how it effects development. And back to that wired story, the actual number of what you might be considering to be maintainers is likely less than 10% of the listed numbers above.
Re: [RFC] MAINTAINERS tag for cleanup robot
On Mon, 2020-11-23 at 09:33 +1100, Finn Thain wrote:
> On Sun, 22 Nov 2020, Joe Perches wrote:
> > But provably correct conversions IMO _should_ be done and IMO churn
> > considerations should generally have less importance.
[]
> Moreover, the patch review workload for skilled humans is being generated
> by the automation, which is completely backwards: the machine is supposed
> to be helping.
Which is why the provably correct matters.
coccinelle transforms can be, but are not necessarily, provably correct.
The _show transforms done via the sysfs_emit_dev.cocci script are correct
as in commit aa838896d87a ("drivers core: Use sysfs_emit and sysfs_emit_at
for show(device *...) functions")
Worthwhile? A different question, but I think yes as it reduces the
overall question space of the existing other sprintf overrun possibilities.
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, 2020-11-22 at 11:12 -0800, James Bottomley wrote: > On Sun, 2020-11-22 at 10:25 -0800, Joe Perches wrote: > > On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote: > > > Please tell me our reward for all this effort isn't a single > > > missing error print. > > > > There were quite literally dozens of logical defects found > > by the fallthrough additions. Very few were logging only. > > So can you give us the best examples (or indeed all of them if someone > is keeping score)? hopefully this isn't a US election situation ... Gustavo? Are you running for congress now? https://lwn.net/Articles/794944/
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote: > Please tell me > our reward for all this effort isn't a single missing error print. There were quite literally dozens of logical defects found by the fallthrough additions. Very few were logging only.
Re: [RFC] MAINTAINERS tag for cleanup robot
On Sun, 2020-11-22 at 08:49 -0800, James Bottomley wrote: > We can enforce sysfs_emit going forwards > using tools like checkpatch It's not really possible for checkpatch to find or warn about sysfs uses of sprintf. checkpatch is really just a trivial line-by-line parser and it has no concept of code intent. It just can't warn on every use of the sprintf family. There are just too many perfectly valid uses. > but there's no benefit and a lot of harm to > be done by trying to churn the entire tree Single uses of sprintf for sysfs is not really any problem. But likely there are still several possible overrun sprintf/snprintf paths in sysfs. Some of them are very obscure and unlikely to be found by a robot as the logic for sysfs buf uses can be fairly twisty. But provably correct conversions IMO _should_ be done and IMO churn considerations should generally have less importance.
Re: [RFC] MAINTAINERS tag for cleanup robot
On Sun, 2020-11-22 at 08:33 -0800, Tom Rix wrote: > On 11/21/20 9:10 AM, Joe Perches wrote: > > On Sat, 2020-11-21 at 08:50 -0800, t...@redhat.com wrote: > > > A difficult part of automating commits is composing the subsystem > > > preamble in the commit log. For the ongoing effort of a fixer producing > > > one or two fixes a release the use of 'treewide:' does not seem > > > appropriate. > > > > > > It would be better if the normal prefix was used. Unfortunately normal is > > > not consistent across the tree. > > > > > > So I am looking for comments for adding a new tag to the MAINTAINERS file > > > > > > D: Commit subsystem prefix > > > > > > ex/ for FPGA DFL DRIVERS > > > > > > D: fpga: dfl: > > I'm all for it. Good luck with the effort. It's not completely trivial. > > > > From a decade ago: > > > > https://lore.kernel.org/lkml/1289919077.28741.50.camel@Joe-Laptop/ > > > > (and that thread started with extra semicolon patches too) > > Reading the history, how about this. > > get_maintainer.pl outputs a single prefix, if multiple files have the > same prefix it works, if they don't its an error. > > Another script 'commit_one_file.sh' does the call to get_mainainter.pl > to get the prefix and be called by run-clang-tools.py to get the fixer > specific message. It's not whether the script used is get_maintainer or any other script, the question is really if the MAINTAINERS file is the appropriate place to store per-subsystem patch specific prefixes. It is. Then the question should be how are the forms described and what is the inheritance priority. My preference would be to have a default of inherit the parent base and add basename(subsystem dirname). Commit history seems to have standardized on using colons as the separator between the commit prefix and the subject. A good mechanism to explore how various subsystems have uses prefixes in the past might be something like: $ git log --no-merges --pretty='%s' - | \ perl -n -e 'print substr($_, 0, rindex($_, ":") + 1) . "\n";' | \ sort | uniq -c | sort -rn Using 1 for commit_count and drivers/scsi for subsystem_path, the top 40 entries are below: About 1% don't have a colon, and there is no real consistency even within individual drivers below scsi. For instance, qla2xxx: 1 814 scsi: qla2xxx: 2 691 scsi: lpfc: 3 389 scsi: hisi_sas: 4 354 scsi: ufs: 5 339 scsi: 6 291 qla2xxx: 7 256 scsi: megaraid_sas: 8 249 scsi: mpt3sas: 9 200 hpsa: 10 190 scsi: aacraid: 11 174 lpfc: 12 153 scsi: qedf: 13 144 scsi: smartpqi: 14 139 scsi: cxlflash: 15 122 scsi: core: 16 110 [SCSI] qla2xxx: 17 108 ncr5380: 18 98 scsi: hpsa: 19 97 20 89 treewide: 21 88 mpt3sas: 22 86 scsi: libfc: 23 85 scsi: qedi: 24 84 scsi: be2iscsi: 25 81 [SCSI] qla4xxx: 26 81 hisi_sas: 27 81 block: 28 75 megaraid_sas: 29 71 scsi: sd: 30 69 [SCSI] hpsa: 31 68 cxlflash: 32 65 scsi: libsas: 33 65 scsi: fnic: 34 61 scsi: scsi_debug: 35 60 scsi: arcmsr: 36 57 be2iscsi: 37 53 atp870u: 38 51 scsi: bfa: 39 50 scsi: storvsc: 40 48 sd:
Re: [PATCH 072/141] can: peak_usb: Fix fall-through warnings for Clang
On Sun, 2020-11-22 at 00:04 +0100, Marc Kleine-Budde wrote: > On 11/21/20 8:50 PM, Joe Perches wrote: > > > What about moving the default to the end if the case, which is more > > > common anyways: > > > > > > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c > > > b/drivers/net/can/usb/peak_usb/pcan_usb_core.c > > [] > > > @@ -295,16 +295,16 @@ static void peak_usb_write_bulk_callback(struct urb > > > *urb) > > > netif_trans_update(netdev); > > > break; > > > > > > > > > - default: > > > - if (net_ratelimit()) > > > - netdev_err(netdev, "Tx urb aborted (%d)\n", > > > - urb->status); > > > case -EPROTO: > > > case -ENOENT: > > > case -ECONNRESET: > > > case -ESHUTDOWN: > > > - > > > break; > > > + > > > + default: > > > + if (net_ratelimit()) > > > + netdev_err(netdev, "Tx urb aborted (%d)\n", > > > + urb->status); > > > > That's fine and is more generally used style but this > > default: case should IMO also end with a break; > > > > + break; > > I don't mind. > > process/coding-style.rst is not totally clear about the break after the > default, > if this is the lase one the switch statement. deprecated.rst has: All switch/case blocks must end in one of: * break; * fallthrough; * continue; * goto ; * return [expression]; I suppose that could be moved into coding-style along with maybe a change to "all switch/case/default blocks"
Re: [PATCH 072/141] can: peak_usb: Fix fall-through warnings for Clang
On Sat, 2020-11-21 at 14:17 +0100, Marc Kleine-Budde wrote: > On 11/20/20 7:34 PM, Gustavo A. R. Silva wrote: > > In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning > > by explicitly adding a break statement instead of letting the code fall > > through to the next case. > > > > Link: https://github.com/KSPP/linux/issues/115 > > Signed-off-by: Gustavo A. R. Silva [] > > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c > > b/drivers/net/can/usb/peak_usb/pcan_usb_core.c [] > > @@ -299,6 +299,8 @@ static void peak_usb_write_bulk_callback(struct urb > > *urb) > > if (net_ratelimit()) > > netdev_err(netdev, "Tx urb aborted (%d)\n", > > urb->status); > > + break; > > + > > case -EPROTO: > > case -ENOENT: > > case -ECONNRESET: > > > > What about moving the default to the end if the case, which is more common > anyways: > > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c > b/drivers/net/can/usb/peak_usb/pcan_usb_core.c [] > @@ -295,16 +295,16 @@ static void peak_usb_write_bulk_callback(struct urb > *urb) > netif_trans_update(netdev); > break; > > > - default: > - if (net_ratelimit()) > - netdev_err(netdev, "Tx urb aborted (%d)\n", > - urb->status); > case -EPROTO: > case -ENOENT: > case -ECONNRESET: > case -ESHUTDOWN: > - > break; > + > + default: > + if (net_ratelimit()) > + netdev_err(netdev, "Tx urb aborted (%d)\n", > + urb->status); That's fine and is more generally used style but this default: case should IMO also end with a break; + break; > }
Re: [RFC] MAINTAINERS tag for cleanup robot
On Sat, 2020-11-21 at 09:18 -0800, James Bottomley wrote: > On Sat, 2020-11-21 at 08:50 -0800, t...@redhat.com wrote: > > A difficult part of automating commits is composing the subsystem > > preamble in the commit log. For the ongoing effort of a fixer > > producing one or two fixes a release the use of 'treewide:' does > > not seem appropriate. > > > > It would be better if the normal prefix was used. Unfortunately > > normal is not consistent across the tree. > > > > D: Commit subsystem prefix > > > > ex/ for FPGA DFL DRIVERS > > > > D: fpga: dfl: > > I've got to bet this is going to cause more issues than it solves. > SCSI uses scsi: : for drivers but not every driver has a > MAINTAINERS entry. We use either scsi: or scsi: core: for mid layer > things, but we're not consistent. Block uses blk-: for all > of it's stuff but almost no s have a MAINTAINERS entry. So > the next thing you're going to cause is an explosion of suggested > MAINTAINERs entries. As well as some changes require simultaneous changes across multiple subsystems. > Has anyone actually complained about treewide:? It depends on what you mean by treewide: If a treewide: patch is applied by some "higher level" maintainer, then generally, no. If the treewide patch is also cc'd to many individual maintainers, then yes, many many times. Mostly because patches cause what is in their view churn or that changes are not specific to their subsystem grounds. The treewide patch is sometimes dropped, sometimes broken up and generally not completely applied. What would be useful in many cases like this is for a pre and post application of the treewide patch to be compiled and the object code verified for lack of any logic change. Unfortunately, gcc does not guarantee deterministic compilation so this isn't feasible with at least gcc. Does clang guarantee this? I'm not sure it's possible: https://blog.llvm.org/2019/11/deterministic-builds-with-clang-and-lld.html
Re: [RFC] MAINTAINERS tag for cleanup robot
On Sat, 2020-11-21 at 08:50 -0800, t...@redhat.com wrote:
> A difficult part of automating commits is composing the subsystem
> preamble in the commit log. For the ongoing effort of a fixer producing
> one or two fixes a release the use of 'treewide:' does not seem appropriate.
>
> It would be better if the normal prefix was used. Unfortunately normal is
> not consistent across the tree.
>
> So I am looking for comments for adding a new tag to the MAINTAINERS file
>
> D: Commit subsystem prefix
>
> ex/ for FPGA DFL DRIVERS
>
> D: fpga: dfl:
I'm all for it. Good luck with the effort. It's not completely trivial.
>From a decade ago:
https://lore.kernel.org/lkml/1289919077.28741.50.camel@Joe-Laptop/
(and that thread started with extra semicolon patches too)
> Continuing with cleaning up clang's -Wextra-semi-stmt
> diff --git a/Makefile b/Makefile
[]
> @@ -1567,20 +1567,21 @@ help:
> echo ''
> @echo 'Static analysers:'
> @echo ' checkstack - Generate a list of stack hogs'
> @echo ' versioncheck- Sanity check on version.h usage'
> @echo ' includecheck- Check for duplicate included header files'
> @echo ' export_report - List the usages of all exported symbols'
> @echo ' headerdep - Detect inclusion cycles in headers'
> @echo ' coccicheck - Check with Coccinelle'
> @echo ' clang-analyzer - Check with clang static analyzer'
> @echo ' clang-tidy - Check with clang-tidy'
> + @echo ' clang-tidy-fix - Check and fix with clang-tidy'
A pity the ordering of the code below isn't the same as the above.
> -PHONY += clang-tidy clang-analyzer
> +PHONY += clang-tidy-fix clang-tidy clang-analyzer
[]
> -clang-tidy clang-analyzer: $(extmod-prefix)compile_commands.json
> +clang-tidy-fix clang-tidy clang-analyzer:
> $(extmod-prefix)compile_commands.json
> $(call cmd,clang_tools)
> else
> -clang-tidy clang-analyzer:
> +clang-tidy-fix clang-tidy clang-analyzer:
[]
> diff --git a/scripts/clang-tools/run-clang-tools.py
> b/scripts/clang-tools/run-clang-tools.py
[]
> @@ -22,43 +22,57 @@ def parse_arguments():
[]
> parser.add_argument("type",
> -choices=["clang-tidy", "clang-analyzer"],
> +choices=["clang-tidy-fix", "clang-tidy",
> "clang-analyzer"],
etc...
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, 2020-11-20 at 12:21 -0600, Gustavo A. R. Silva wrote: > Hi all, > > This series aims to fix almost all remaining fall-through warnings in > order to enable -Wimplicit-fallthrough for Clang. > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > add multiple break/goto/return/fallthrough statements instead of just > letting the code fall through to the next case. > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this > change[1] is meant to be reverted at some point. So, this patch helps > to move in that direction. This was a bit hard to parse for a second or three. Thanks Gustavo. How was this change done?
Re: [PATCH v2] mdio_bus: suppress err message for reset gpio EPROBE_DEFER
On Thu, 2020-11-19 at 21:21 -0800, Jakub Kicinski wrote: > We do have our own comment style rule in networking since the beginning > of time, and reverse xmas tree, so it's not completely crazy. reverse xmas tree is completely crazy. But I posted a patch to checkpatch to suggest it for net/ and drivers/net/ once https://lkml.org/lkml/2016/11/4/54
Re: [PATCH net-next] MAINTAINERS: Update XDP and AF_XDP entries
On Thu, 2020-11-19 at 21:50 +0100, Jesper Dangaard Brouer wrote: > On Thu, 19 Nov 2020 10:02:10 -0800 > Jakub Kicinski wrote: > > > On Thu, 19 Nov 2020 18:26:40 +0100 Jesper Dangaard Brouer wrote: > > > Getting too many false positive matches with current use > > > of the content regex K: and file regex N: patterns. > > > > > > This patch drops file match N: and makes K: more restricted. > > > Some more normal F: file wildcards are added. > > > > > > Notice that AF_XDP forgot to some F: files that is also > > > updated in this patch. > > > > > > Suggested-by: Jakub Kicinski > > > Signed-off-by: Jesper Dangaard Brouer > > > > Ah! Sorry, I missed that you sent this before replying to Joe. > > > > Would you mind respining with his regex? > > Sure, I just send it... with your adjusted '(\b|_)xdp(\b|_)' regex, as > it seems to do the same thing (and it works with egrep). The regexes in MAINTAINERS are perl not egrep and using (\b|_) creates unnecessary capture groups. It _really_ should be (?:\b|_)xdp(?:\b|_)
Re: XDP maintainer match (Was [PATCH v2 0/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring)
On Thu, 2020-11-19 at 17:35 +0100, Jesper Dangaard Brouer wrote: > On Thu, 19 Nov 2020 07:46:34 -0800 Jakub Kicinski wrote: > I think it is a good idea to change the keyword (K:), but I'm not sure > this catch what we want, maybe it does. The pattern match are meant to > catch drivers containing XDP related bits. > > Previously Joe Perches suggested this pattern match, > which I don't fully understand... could you explain Joe? > > (?:\b|_)xdp(?:\b|_) This regex matches only: xdp xdp_ _xdp_ _xdp > For the filename (N:) regex match, I'm considering if we should remove > it and list more files explicitly. I think normal glob * pattern > works, which should be sufficient. Lists are generally more specific than regex globs.
Re: [PATCH] net/core: use xx_zalloc instead xx_alloc and memset
On Wed, 2020-11-18 at 16:15 +0800, Tian Tao wrote:
> use kmem_cache_zalloc instead kmem_cache_alloc and memset.
[]
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
[]
> @@ -313,12 +313,10 @@ struct sk_buff *__build_skb(void *data, unsigned int
> frag_size)
> {
> struct sk_buff *skb;
>
> - skb = kmem_cache_alloc(skbuff_head_cache, GFP_ATOMIC);
> + skb = kmem_cache_zalloc(skbuff_head_cache, GFP_ATOMIC);
> if (unlikely(!skb))
> return NULL;
>
> - memset(skb, 0, offsetof(struct sk_buff, tail));
> -
> return __build_skb_around(skb, data, frag_size);
> }
>
>
> @@ -6170,12 +6168,10 @@ static void *skb_ext_get_ptr(struct skb_ext *ext,
> enum skb_ext_id id)
> */
> struct skb_ext *__skb_ext_alloc(gfp_t flags)
> {
> - struct skb_ext *new = kmem_cache_alloc(skbuff_ext_cache, flags);
> + struct skb_ext *new = kmem_cache_zalloc(skbuff_ext_cache, flags);
>
> - if (new) {
> - memset(new->offset, 0, sizeof(new->offset));
> + if (new)
> refcount_set(&new->refcnt, 1);
> - }
>
> return new;
> }
I think it'd be nicer to use the same form for both of these functions.
This could be:
struct skb_ext *__skb_ext_alloc(gfp_t flags)
{
struct skb_ext *new;
new = kmem_cache_zalloc(skbbuff_ext_cache, flags);
if (unlikely(!new))
return NULL;
refcount_set(&new->refcnt, 1);
return new;
}
Re: [PATCH][next] mwifiex: Fix fall-through warnings for Clang
On Tue, 2020-11-17 at 10:09 -0600, Gustavo A. R. Silva wrote: > In preparation to enable -Wimplicit-fallthrough for Clang, fix multiple > warnings by explicitly adding multiple break statements instead of > letting the code fall through to the next case. Thanks Gustavo. I think this is better style than the gcc allowed undescribed fallthrough to break; gcc developers disagree though: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91432
Re: [PATCH] [v7] wireless: Initial driver submission for pureLiFi STA devices
On Mon, 2020-11-16 at 14:52 +0530, Srinivasan Raju wrote:
> This introduces the pureLiFi LiFi driver for LiFi-X, LiFi-XC
> and LiFi-XL USB devices.
>
> This driver implementation has been based on the zd1211rw driver.
>
> Driver is based on 802.11 softMAC Architecture and uses
> native 802.11 for configuration and management.
>
> The driver is compiled and tested in ARM, x86 architectures and
> compiled in powerpc architecture.
>
> Signed-off-by: Srinivasan Raju
trivial notes and some style and content defects:
(I stopped reading after awhile)
Commonly this changelog would go below the --- separator line.
>
> Changes v6->v7:
> - Magic numbers removed and used IEEE80211 macors
> - usb.c is split into two files firmware.c and dbgfs.c
> - Other code style and timer function fixes (mod_timer)
> Changes v5->v6:
> - Code style fix patch from Joe Perches
> Changes v4->v5:
> - Code refactoring for clarity and redundnacy removal
> - Fix warnings from kernel test robot
> Changes v3->v4:
> - Code refactoring based on kernel code guidelines
> - Remove multi level macors and use kernel debug macros
> Changes v2->v3:
> - Code style fixes kconfig fix
> Changes v1->v2:
> - v1 was submitted to staging, v2 submitted to wireless-next
> - Code style fixes and copyright statement fix
> ---
> MAINTAINERS |5 +
[]
> diff --git a/MAINTAINERS b/MAINTAINERS
[]
> @@ -14108,6 +14108,11 @@ T: git git://linuxtv.org/media_tree.git
[]
> +PUREILIFI USB DRIVER
> +M: Srinivasan Raju
> +S: Maintained
If you are employed there and are paid to maintain this code the
more common S: marking is "Supported"
> diff --git a/drivers/net/wireless/purelifi/Kconfig
> b/drivers/net/wireless/purelifi/Kconfig
[]
> +++ b/drivers/net/wireless/purelifi/Kconfig
> @@ -0,0 +1,27 @@
> +# SPDX-License-Identifier: GPL-2.0
For clarity, I think it'd be nicer to use GPL-2.0-only here and elsewhere.
> diff --git a/drivers/net/wireless/purelifi/chip.c
> b/drivers/net/wireless/purelifi/chip.c
[]
> +int purelifi_set_beacon_interval(struct purelifi_chip *chip, u16 interval,
> + u8 dtim_period, int type)
> +{
> + if (!interval || (chip->beacon_set &&
> + chip->beacon_interval == interval)) {
> + return 0;
> + }
It's ddd that checkpatch isn't complaining about single statement uses
with braces. I would write this like the below, but there isn't really
anything wrong with what you did either.
if (!interval ||
(chip->beacon_set && chip->beacon_interval == interval))
return 0;
> +void purelifi_chip_disable_rxtx(struct purelifi_chip *chip)
> +{
> + u8 value;
> +
> + value = 0;
why not make this:
static const u8 value = 0;
> + usb_write_req((const u8 *)&value, sizeof(value), USB_REQ_RXTX_WR);
so this is doesn't need a cast
usb_write_req(&value, sizeof(value), USB_REQ_RXTX_WR);
> +int purelifi_chip_set_rate(struct purelifi_chip *chip, u8 rate)
> +{
> + int r;
> +
> + if (!chip)
> + return -EINVAL;
> +
> + r = usb_write_req((const u8 *)&rate, sizeof(rate), USB_REQ_RATE_WR);
why is the cast needed here?
> +static inline void purelifi_mc_add_addr(struct purelifi_mc_hash *hash,
> + u8 *addr
> + )
Odd close parenthesis location
> diff --git a/drivers/net/wireless/purelifi/dbgfs.c
> b/drivers/net/wireless/purelifi/dbgfs.c
[]
> +ssize_t purelifi_store_frequency(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t len)
> +{
[]
> + if (valid) {
> + usr_input[0] = (char)predivider;
> + usr_input[1] = (char)feedback_divider;
> + usr_input[2] = (char)output_div_0;
> + usr_input[3] = (char)output_div_1;
> + usr_input[4] = (char)output_div_2;
> + usr_input[5] = (char)clkout3_phase;
> +
> + dev_err(dev, "options IP_DIV: %d VCO_MULT: %d OP_DIV_PHY: %d",
> + usr_input[0], usr_input[1], usr_input[2]);
> + dev_err(dev, "OP_DIV_CPU: %d OP_DIV_USB: %d CLK3_PH: %d\n",
> + usr_input[3], usr_input[4], usr_input[5]);
why is this dev_err? It's not an error.
Shouldn't this be dev_notice or dev_info?
> +ssize_t purelifi_show_sysfs(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> +
Re: [PATCH] netfilter: conntrack: fix -Wformat
On Tue, 2020-11-10 at 14:00 -0800, Nick Desaulniers wrote: > Yeah, we could go through and remove %h and %hh to solve this, too, right? Yup. I think one of the checkpatch improvement mentees is adding some suggestion and I hope an automated fix mechanism for that. https://lore.kernel.org/lkml/5e3265c241602bb54286fbaae9222070daa4768e.ca...@perches.com/
Re: Subject: [RFC] clang tooling cleanups
On Tue, 2020-10-27 at 09:42 -0700, t...@redhat.com wrote:
> This rfc will describe
> An upcoming treewide cleanup.
> How clang tooling was used to programatically do the clean up.
> Solicit opinions on how to generally use clang tooling.
>
> The clang warning -Wextra-semi-stmt produces about 10k warnings.
> Reviewing these, a subset of semicolon after a switch looks safe to
> fix all the time. An example problem
>
> void foo(int a) {
> switch(a) {
> case 1:
> ...
> }; <--- extra semicolon
> }
>
> Treewide, there are about 100 problems in 50 files for x86_64 allyesconfig.
> These fixes will be the upcoming cleanup.
coccinelle already does some of these.
For instance: scripts/coccinelle/misc/semicolon.cocci
Perhaps some tool coordination can be done here as
coccinelle/checkpatch/clang/Lindent call all be used
to do some facet or another of these cleanup issues.
Re: [PATCH] netfilter: conntrack: fix -Wformat
On Sun, 2020-11-08 at 08:34 +0100, Lukas Bulwahn wrote:
> On Sat, 7 Nov 2020, Joe Perches wrote:
> > On Fri, 2020-11-06 at 23:55 -0800, Nick Desaulniers wrote:
> > > Clang is more aggressive about -Wformat warnings when the format flag
> > > specifies a type smaller than the parameter. Fixes 8 instances of:
> > >
> > > warning: format specifies type 'unsigned short' but the argument has
> > > type 'int' [-Wformat]
> >
> > Likely clang's -Wformat message is still bogus.
> > Wasn't that going to be fixed?
> >
> > Integer promotions are already done on these types to int anyway.
> > Didn't we have this discussion last year?
> >
> > https://lore.kernel.org/lkml/CAKwvOd=mqzj2pAZEUsW-M_62xn4pijpCJmP=b1h_-web0ne...@mail.gmail.com/
> > https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20...@mail.gmail.com/
> > https://lore.kernel.org/lkml/a68114afb134b8633905f5a25ae7c4e6799ce8f1.ca...@perches.com/
> >
> > Look at commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use
> > of unnecessary %h[xudi] and %hh[xudi]")
> >
> > The "h" and "hh" things should never be used. The only reason for them
> > being used if if you have an "int", but you want to print it out as a
> > "char" (and honestly, that is a really bad reason, you'd be better off
> > just using a proper cast to make the code more obvious).
> >
> Joe, would this be a good rule to check for in checkpatch?
>
> Can Dwaipayan or Aditya give it a try to create a suitable patch to add
> such a rule?
$ git grep -P '"[^"]*%[\d\.\*\-]*h+[idux].*"'
I suppose so.
Please avoid warning on scanf and its variants and the asm bits though.
> Dwaipayan, Aditya, if Joe thinks it is worth a rule, it is "first come,
> first serve" for you to take that task.
Re: [PATCH] net/dsa: remove unused macros to tame gcc warning
On Sat, 2020-11-07 at 20:54 +0800, Alex Shi wrote: > 在 2020/11/7 上午12:39, Florian Fainelli 写道: > > > It is good to remember that there are multiple readers of source > > > files. There is the compiler which generates code from it, and there > > > is the human trying to understand what is going on, what the hardware > > > can do, how we could maybe extend the code in the future to make use > > > of bits are currently don't, etc. > > > > > > The compiler has no use of these macros, at the moment. But i as a > > > human do. It is valuable documentation, given that there is no open > > > datasheet for this hardware. > > > > > > I would say these warnings are bogus, and the code should be left > > > alone. > > Agreed, these definitions are intended to document what the hardware > > does. These warnings are getting too far. > > Thanks for all comments! I agree these info are much meaningful. > Is there other way to tame the gcc warning? like put them into a .h file > or covered by comments? Does _any_ version of gcc have this warning on by default? I still think my proposal of moving the warning from W=2 to W=3 quite reasonable. Another possibility is to turn the warning off altogether.
Re: [PATCH] netfilter: conntrack: fix -Wformat
On Fri, 2020-11-06 at 23:55 -0800, Nick Desaulniers wrote:
> Clang is more aggressive about -Wformat warnings when the format flag
> specifies a type smaller than the parameter. Fixes 8 instances of:
>
> warning: format specifies type 'unsigned short' but the argument has
> type 'int' [-Wformat]
Likely clang's -Wformat message is still bogus.
Wasn't that going to be fixed?
Integer promotions are already done on these types to int anyway.
Didn't we have this discussion last year?
https://lore.kernel.org/lkml/CAKwvOd=mqzj2pAZEUsW-M_62xn4pijpCJmP=b1h_-web0ne...@mail.gmail.com/
https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20...@mail.gmail.com/
https://lore.kernel.org/lkml/a68114afb134b8633905f5a25ae7c4e6799ce8f1.ca...@perches.com/
Look at commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use
of unnecessary %h[xudi] and %hh[xudi]")
The "h" and "hh" things should never be used. The only reason for them
being used if if you have an "int", but you want to print it out as a
"char" (and honestly, that is a really bad reason, you'd be better off
just using a proper cast to make the code more obvious).
So if what you have a "char" (or unsigned char) you should always just
print it out as an "int", knowing that the compiler already did the
proper type conversion.
> diff --git a/net/netfilter/nf_conntrack_standalone.c
> b/net/netfilter/nf_conntrack_standalone.c
[]
> @@ -50,38 +50,38 @@ print_tuple(struct seq_file *s, const struct
> nf_conntrack_tuple *tuple,
>
>
> switch (l4proto->l4proto) {
> case IPPROTO_ICMP:
> - seq_printf(s, "type=%u code=%u id=%u ",
> + seq_printf(s, "type=%u code=%u id=%hu ",
etc...
Re: [PATCH] libbpf: Remove unnecessary conversion to bool
On Fri, 2020-11-06 at 13:32 -0800, Andrii Nakryiko wrote:
> On Thu, Nov 5, 2020 at 11:12 PM wrote:
> > Fix following warning from coccinelle:
> > ./tools/lib/bpf/libbpf.c:1478:43-48: WARNING: conversion to bool not needed
> > here
[]
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
[]
> > @@ -1475,7 +1475,7 @@ static int set_kcfg_value_tri(struct extern_desc
> > *ext, void *ext_val,
> > ext->name, value);
> > return -EINVAL;
> > }
> > - *(bool *)ext_val = value == 'y' ? true : false;
> > + *(bool *)ext_val = value == 'y';
>
> I actually did this intentionally. x = y == z; pattern looked too
> obscure to my taste, tbh.
It's certainly a question of taste and obviously there is nothing
wrong with yours.
Maybe adding parentheses makes the below look less obscure to you?
x = (y == z);
My taste would run to something like:
---
tools/lib/bpf/libbpf.c | 25 +
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 313034117070..5d9c9c8d50c9 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1469,25 +1469,34 @@ static int set_kcfg_value_tri(struct extern_desc *ext,
void *ext_val,
char value)
{
switch (ext->kcfg.type) {
- case KCFG_BOOL:
+ case KCFG_BOOL: {
+ bool *p = ext_val;
+
if (value == 'm') {
pr_warn("extern (kcfg) %s=%c should be tristate or
char\n",
ext->name, value);
return -EINVAL;
}
- *(bool *)ext_val = value == 'y' ? true : false;
+ *p = (value == 'y');
break;
- case KCFG_TRISTATE:
+ }
+ case KCFG_TRISTATE: {
+ enum libbpf_tristate *p = ext_val;
+
if (value == 'y')
- *(enum libbpf_tristate *)ext_val = TRI_YES;
+ *p = TRI_YES;
else if (value == 'm')
- *(enum libbpf_tristate *)ext_val = TRI_MODULE;
+ *p = TRI_MODULE;
else /* value == 'n' */
- *(enum libbpf_tristate *)ext_val = TRI_NO;
+ *p = TRI_NO;
break;
- case KCFG_CHAR:
- *(char *)ext_val = value;
+ }
+ case KCFG_CHAR: {
+ char *p = ext_val;
+
+ *p = value;
break;
+ }
case KCFG_UNKNOWN:
case KCFG_INT:
case KCFG_CHAR_ARR:
Re: [PATCH] net/dsa: remove unused macros to tame gcc warning
On Fri, 2020-11-06 at 16:28 +0800, Alex Shi wrote: > > 在 2020/11/6 下午2:36, Joe Perches 写道: > > On Fri, 2020-11-06 at 13:37 +0800, Alex Shi wrote: > > > There are some macros unused, they causes much gcc warnings. Let's > > > remove them to tame gcc. > > > > I believe these to be essentially poor warnings. > > > > Aren't these warnings generated only when adding W=2 to the make > > command line? > > > > Perhaps it's better to move the warning to level 3 > > --- > > diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn > > index 95e4cdb94fe9..5c3c220ddb32 100644 > > --- a/scripts/Makefile.extrawarn > > +++ b/scripts/Makefile.extrawarn > > @@ -68,7 +68,6 @@ KBUILD_CFLAGS += $(call cc-option, -Wlogical-op) > > KBUILD_CFLAGS += -Wmissing-field-initializers > > KBUILD_CFLAGS += -Wtype-limits > > KBUILD_CFLAGS += $(call cc-option, -Wmaybe-uninitialized) > > -KBUILD_CFLAGS += $(call cc-option, -Wunused-macros) > > This changed too much, and impact others. May not good. :) Can you clarify what you mean?
Re: [PATCH] net/dsa: remove unused macros to tame gcc warning
On Fri, 2020-11-06 at 13:37 +0800, Alex Shi wrote: > There are some macros unused, they causes much gcc warnings. Let's > remove them to tame gcc. I believe these to be essentially poor warnings. Aren't these warnings generated only when adding W=2 to the make command line? Perhaps it's better to move the warning to level 3 --- diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn index 95e4cdb94fe9..5c3c220ddb32 100644 --- a/scripts/Makefile.extrawarn +++ b/scripts/Makefile.extrawarn @@ -68,7 +68,6 @@ KBUILD_CFLAGS += $(call cc-option, -Wlogical-op) KBUILD_CFLAGS += -Wmissing-field-initializers KBUILD_CFLAGS += -Wtype-limits KBUILD_CFLAGS += $(call cc-option, -Wmaybe-uninitialized) -KBUILD_CFLAGS += $(call cc-option, -Wunused-macros) KBUILD_CPPFLAGS += -DKBUILD_EXTRA_WARN2 @@ -89,6 +88,7 @@ KBUILD_CFLAGS += -Wredundant-decls KBUILD_CFLAGS += -Wsign-compare KBUILD_CFLAGS += -Wswitch-default KBUILD_CFLAGS += $(call cc-option, -Wpacked-bitfield-compat) +KBUILD_CFLAGS += $(call cc-option, -Wunused-macros) KBUILD_CPPFLAGS += -DKBUILD_EXTRA_WARN3
Re: [PATCH] cxgb4: Fix the -Wmisleading-indentation warning
On Wed, 2020-11-04 at 13:24 +0800, xiakaixu1...@gmail.com wrote:
> From: Kaixu Xia
>
> Fix the gcc warning:
>
> drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c:2673:9: warning: this
> 'for' clause does not guard... [-Wmisleading-indentation]
> 2673 | for (i = 0; i < n; ++i) \
true, the defined macros though aren't pretty and depend on
externally defined i and n.
It'd be good to show that and to update the slightly difficult to read
helpers below that and remove the unnecessary T3 and R3 macros too.
Perhaps:
---
drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 28 ++
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
index 0273f40b85f7..a7fddcdf4eac 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
@@ -2666,20 +2666,20 @@ static int sge_qinfo_show(struct seq_file *seq, void *v)
if (r)
seq_putc(seq, '\n');
-#define S3(fmt_spec, s, v) \
-do { \
- seq_printf(seq, "%-12s", s); \
- for (i = 0; i < n; ++i) \
- seq_printf(seq, " %16" fmt_spec, v); \
- seq_putc(seq, '\n'); \
+/* These macros are dependent on locally scoped i and n variables */
+#define S3(fmt_spec, s, v) \
+do { \
+ seq_printf(seq, "%-12s", s);\
+ for (i = 0; i < n; ++i) \
+ seq_printf(seq, " %16" fmt_spec, v);\
+ seq_putc(seq, '\n');\
} while (0)
-#define S(s, v) S3("s", s, v)
-#define T3(fmt_spec, s, v) S3(fmt_spec, s, tx[i].v)
-#define T(s, v) S3("u", s, tx[i].v)
-#define TL(s, v) T3("lu", s, v)
-#define R3(fmt_spec, s, v) S3(fmt_spec, s, rx[i].v)
-#define R(s, v) S3("u", s, rx[i].v)
-#define RL(s, v) R3("lu", s, v)
+
+#define S(s, v)S3("s", s, v)
+#define T(s, v)S3("u", s, tx[i].v)
+#define TL(s, v) S3("lu", s, tx[i].v)
+#define R(s, v)S3("u", s, rx[i].v)
+#define RL(s, v) S3("lu", s, rx[i].v)
if (r < eth_entries) {
int base_qset = r * 4;
@@ -3139,8 +3139,6 @@ do { \
#undef T
#undef TL
#undef S
-#undef R3
-#undef T3
#undef S3
out:
return 0;
Re: [PATCH v2 0/8] slab: provide and use krealloc_array()
On Mon, 2020-11-02 at 16:20 +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski > > Andy brought to my attention the fact that users allocating an array of > equally sized elements should check if the size multiplication doesn't > overflow. This is why we have helpers like kmalloc_array(). > > However we don't have krealloc_array() equivalent and there are many > users who do their own multiplication when calling krealloc() for arrays. > > This series provides krealloc_array() and uses it in a couple places. My concern about this is a possible assumption that __GFP_ZERO will work, and as far as I know, it will not.
Re: [PATCH 3/8] vhost: vringh: use krealloc_array()
On Tue, 2020-10-27 at 17:58 +0100, Bartosz Golaszewski wrote:
> On Tue, Oct 27, 2020 at 5:50 PM Joe Perches wrote:
> >
> > On Tue, 2020-10-27 at 11:28 -0400, Michael S. Tsirkin wrote:
> > > On Tue, Oct 27, 2020 at 01:17:20PM +0100, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski
> > > >
> > > > Use the helper that checks for overflows internally instead of manually
> > > > calculating the size of the new array.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski
> > >
> > > No problem with the patch, it does introduce some symmetry in the code.
> >
> > Perhaps more symmetry by using kmemdup
> > ---
> > drivers/vhost/vringh.c | 23 ++-
> > 1 file changed, 10 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > index 8bd8b403f087..99222a3651cd 100644
> > --- a/drivers/vhost/vringh.c
> > +++ b/drivers/vhost/vringh.c
> > @@ -191,26 +191,23 @@ static int move_to_indirect(const struct vringh *vrh,
> > static int resize_iovec(struct vringh_kiov *iov, gfp_t gfp)
> > {
> > struct kvec *new;
> > - unsigned int flag, new_num = (iov->max_num & ~VRINGH_IOV_ALLOCATED)
> > * 2;
> > + size_t new_num = (iov->max_num & ~VRINGH_IOV_ALLOCATED) * 2;
> > + size_t size;
> >
> > if (new_num < 8)
> > new_num = 8;
> >
> > - flag = (iov->max_num & VRINGH_IOV_ALLOCATED);
> > - if (flag)
> > - new = krealloc(iov->iov, new_num * sizeof(struct iovec),
> > gfp);
> > - else {
> > - new = kmalloc_array(new_num, sizeof(struct iovec), gfp);
> > - if (new) {
> > - memcpy(new, iov->iov,
> > - iov->max_num * sizeof(struct iovec));
> > - flag = VRINGH_IOV_ALLOCATED;
> > - }
> > - }
> > + if (unlikely(check_mul_overflow(new_num, sizeof(struct iovec),
> > &size)))
> > + return -ENOMEM;
> > +
>
> The whole point of using helpers such as kmalloc_array() is not doing
> these checks manually.
Tradeoffs for in readability for overflow and not mistyping or doing
the multiplication of iov->max_num * sizeof(struct iovec) twice.
Just fyi:
the realloc doesn't do a multiplication overflow test as written so the
suggestion is slightly more resistant to defect.
Re: [PATCH 3/8] vhost: vringh: use krealloc_array()
On Tue, 2020-10-27 at 11:28 -0400, Michael S. Tsirkin wrote:
> On Tue, Oct 27, 2020 at 01:17:20PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski
> >
> > Use the helper that checks for overflows internally instead of manually
> > calculating the size of the new array.
> >
> > Signed-off-by: Bartosz Golaszewski
>
> No problem with the patch, it does introduce some symmetry in the code.
Perhaps more symmetry by using kmemdup
---
drivers/vhost/vringh.c | 23 ++-
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
index 8bd8b403f087..99222a3651cd 100644
--- a/drivers/vhost/vringh.c
+++ b/drivers/vhost/vringh.c
@@ -191,26 +191,23 @@ static int move_to_indirect(const struct vringh *vrh,
static int resize_iovec(struct vringh_kiov *iov, gfp_t gfp)
{
struct kvec *new;
- unsigned int flag, new_num = (iov->max_num & ~VRINGH_IOV_ALLOCATED) * 2;
+ size_t new_num = (iov->max_num & ~VRINGH_IOV_ALLOCATED) * 2;
+ size_t size;
if (new_num < 8)
new_num = 8;
- flag = (iov->max_num & VRINGH_IOV_ALLOCATED);
- if (flag)
- new = krealloc(iov->iov, new_num * sizeof(struct iovec), gfp);
- else {
- new = kmalloc_array(new_num, sizeof(struct iovec), gfp);
- if (new) {
- memcpy(new, iov->iov,
- iov->max_num * sizeof(struct iovec));
- flag = VRINGH_IOV_ALLOCATED;
- }
- }
+ if (unlikely(check_mul_overflow(new_num, sizeof(struct iovec), &size)))
+ return -ENOMEM;
+
+ if (iov->max_num & VRINGH_IOV_ALLOCATED)
+ new = krealloc(iov->iov, size, gfp);
+ else
+ new = kmemdup(iov->iov, size, gfp);
if (!new)
return -ENOMEM;
iov->iov = new;
- iov->max_num = (new_num | flag);
+ iov->max_num = new_num | VRINGH_IOV_ALLOCATED;
return 0;
}
Re: [PATCH -next] neigh: remove the extra slash
On Fri, 2020-10-23 at 13:16 +0300, Vasily Averin wrote:
> On 10/23/20 1:01 PM, Zhang Qilong wrote:
> > The normal path has only one slash.
>
> it is not normal path
> this string is used to calculate number of symbols in "net/%s/neigh/%s" used
> below
Then probably better would be to add +1 rather than
use a rather odd filename.
> > diff --git a/net/core/neighbour.c b/net/core/neighbour.c
[]
> > @@ -3623,7 +3623,7 @@ int neigh_sysctl_register(struct net_device *dev,
> > struct neigh_parms *p,
> > int i;
> > struct neigh_sysctl_table *t;
> > const char *dev_name_source;
> > - char neigh_path[ sizeof("net//neigh/") + IFNAMSIZ + IFNAMSIZ ];
> > + char neigh_path[sizeof("net/neigh/") + IFNAMSIZ + IFNAMSIZ];
> > char *p_name;
> >
> >
> > t = kmemdup(&neigh_sysctl_template, sizeof(*t), GFP_KERNEL);
> >
Re: [RFC] treewide: cleanup unreachable breaks
On Mon, 2020-10-19 at 12:42 -0700, Nick Desaulniers wrote: > On Sat, Oct 17, 2020 at 10:43 PM Greg KH wrote: > > On Sat, Oct 17, 2020 at 09:09:28AM -0700, t...@redhat.com wrote: > > > From: Tom Rix > > > > > > This is a upcoming change to clean up a new warning treewide. > > > I am wondering if the change could be one mega patch (see below) or > > > normal patch per file about 100 patches or somewhere half way by > > > collecting > > > early acks. > > > > Please break it up into one-patch-per-subsystem, like normal, and get it > > merged that way. > > > > Sending us a patch, without even a diffstat to review, isn't going to > > get you very far... > > Tom, > If you're able to automate this cleanup, I suggest checking in a > script that can be run on a directory. Then for each subsystem you > can say in your commit "I ran scripts/fix_whatever.py on this subdir." > Then others can help you drive the tree wide cleanup. Then we can > enable -Wunreachable-code-break either by default, or W=2 right now > might be a good idea. > > Ah, George (gbiv@, cc'ed), did an analysis recently of > `-Wunreachable-code-loop-increment`, `-Wunreachable-code-break`, and > `-Wunreachable-code-return` for Android userspace. From the review: > ``` > Spoilers: of these, it seems useful to turn on > -Wunreachable-code-loop-increment and -Wunreachable-code-return by > default for Android > ... > While these conventions about always having break arguably became > obsolete when we enabled -Wfallthrough, my sample turned up zero > potential bugs caught by this warning, and we'd need to put a lot of > effort into getting a clean tree. So this warning doesn't seem to be > worth it. > ``` > Looks like there's an order of magnitude of `-Wunreachable-code-break` > than the other two. > > We probably should add all 3 to W=2 builds (wrapped in cc-option). > I've filed https://github.com/ClangBuiltLinux/linux/issues/1180 to > follow up on. I suggest using W=1 as people that are doing cleanups generally use that and not W=123 or any other style. Every other use of W= is still quite noisy and these code warnings are relatively trivially to fix up.
Re: [PATCH] [v6] wireless: Initial driver submission for pureLiFi STA devices
On Mon, 2020-10-19 at 16:40 +, Srinivasan Raju wrote: > > Overall, there are many magic numbers without comments, this makes it hard > > to > > understand the code. Using defines with proper naming helps and for 802.11 > > stuff > > can use ieee80211_*/IEEE80211_* should be used. > > Thanks for your comments Krishna, will work on them. When you do, please start adding changelog information below the --- line to your patches. It's quite a chore to figure out what changed between revisions.
Re: [PATCH] wireless: remove unneeded break
On Mon, 2020-10-19 at 10:54 -0500, Gustavo A. R. Silva wrote:
> On 10/19/20 10:21, Joe Perches wrote:
> > On Mon, 2020-10-19 at 17:14 +0200, Christian Lamparter wrote:
> > > On 19/10/2020 17:05, t...@redhat.com wrote:
> > > > From: Tom Rix
> > > >
> > > > A break is not needed if it is preceded by a return or goto
> > > >
> > > > Signed-off-by: Tom Rix
> > > > diff --git a/drivers/net/wireless/intersil/p54/eeprom.c
> > > > b/drivers/net/wireless/intersil/p54/eeprom.c
[]
> > > > @@ -870,7 +870,6 @@ int p54_parse_eeprom(struct ieee80211_hw *dev, void
> > > > *eeprom, int len)
> > > > } else {
> > > > goto good_eeprom;
> > > > }
> > > > - break;
> > > Won't the compiler (gcc) now complain about a missing fallthrough
> > > annotation?
>
> Clang would definitely complain about this.
As far as I can tell, clang 10.0.0 doesn't complain.
This compiles without fallthrough complaint
from make V=1 W=123 CC=clang drivers/net/wireless/intersil/p54/eeprom.o
with -Wimplicit-fallthrough added
$ clang -Wp,-MMD,drivers/net/wireless/intersil/p54/.eeprom.o.d -nostdinc
-isystem /usr/local/lib/clang/10.0.0/include -I./arch/x86/include
-I./arch/x86/include/generated -I./include -I./arch/x86/include/uapi
-I./arch/x86/include/generated/uapi -I./include/uapi -I./include/generated/uapi
-include ./include/linux/kconfig.h -include ./include/linux/compiler_types.h
-D__KERNEL__ -Qunused-arguments -DKBUILD_EXTRA_WARN1 -DKBUILD_EXTRA_WARN2
-DKBUILD_EXTRA_WARN3 -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs
-fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE
-Werror=implicit-function-declaration -Werror=implicit-int -Wno-format-security
-std=gnu89 -no-integrated-as -Werror=unknown-warning-option -mno-sse -mno-mmx
-mno-sse2 -mno-3dnow -mno-avx -m64 -mno-80387 -mstack-alignment=8
-mtune=generic -mno-red-zone -mcmodel=kernel -DCONFIG_X86_X32_ABI
-Wno-sign-compare -fno-asynchronous-unwind-tables -mretpoline-external-thunk
-fno-delete-null-pointer-checks -Wno-address-of-packed-member -O2
-Wframe-larger-than=2048 -fstack-protector-strong -Wno-format-invalid-specifier
-Wno-gnu -mno-global-merge -Wno-unused-const-variable
-ftrivial-auto-var-init=pattern -pg -mfentry -DCC_USING_FENTRY
-falign-functions=32 -Wdeclaration-after-statement -Wvla -Wno-pointer-sign
-Wno-array-bounds -fno-strict-overflow -fno-stack-check -Werror=date-time
-Werror=incompatible-pointer-types -fcf-protection=none -Wextra -Wunused
-Wno-unused-parameter -Wmissing-declarations -Wmissing-format-attribute
-Wmissing-prototypes -Wold-style-definition -Wmissing-include-dirs
-Wunused-const-variable -Wno-missing-field-initializers -Wno-sign-compare
-Wno-type-limits -Wcast-align -Wdisabled-optimization -Wnested-externs -Wshadow
-Wmissing-field-initializers -Wtype-limits -Wunused-macros -Wbad-function-cast
-Wcast-qual -Wconversion -Wpacked -Wpadded -Wpointer-arith -Wredundant-decls
-Wsign-compare -Wswitch-default -fsanitize=kernel-address -mllvm
-asan-mapping-offset=0xdc00 -mllvm -asan-globals=1 -mllvm
-asan-instrumentation-with-call-threshold=0 -mllvm -asan-stack=0 --param
asan-instrument-allocas=1 -fsanitize-coverage=trace-pc
-fsanitize-coverage=trace-cmp -Wimplicit-fallthrough
-DKBUILD_MODFILE='"drivers/net/wireless/intersil/p54/p54common"'
-DKBUILD_BASENAME='"eeprom"' -DKBUILD_MODNAME='"p54common"' -c -o
drivers/net/wireless/intersil/p54/eeprom.o
drivers/net/wireless/intersil/p54/eeprom.c
Re: [PATCH] wireless: remove unneeded break
On Mon, 2020-10-19 at 17:14 +0200, Christian Lamparter wrote:
> On 19/10/2020 17:05, t...@redhat.com wrote:
> > From: Tom Rix
> >
> > A break is not needed if it is preceded by a return or goto
> >
> > Signed-off-by: Tom Rix
> > diff --git a/drivers/net/wireless/intersil/p54/eeprom.c
> > b/drivers/net/wireless/intersil/p54/eeprom.c
> > index 5bd35c147e19..3ca9d26df174 100644
> > --- a/drivers/net/wireless/intersil/p54/eeprom.c
> > +++ b/drivers/net/wireless/intersil/p54/eeprom.c
> > @@ -870,7 +870,6 @@ int p54_parse_eeprom(struct ieee80211_hw *dev, void
> > *eeprom, int len)
> > } else {
> > goto good_eeprom;
> > }
> > - break;
> Won't the compiler (gcc) now complain about a missing fallthrough annotation?
> > default:
> > break;
> > }
No, though the code would be clearer like:
---
diff --git a/drivers/net/wireless/intersil/p54/eeprom.c
b/drivers/net/wireless/intersil/p54/eeprom.c
index 5bd35c147e19..233fa072d96d 100644
--- a/drivers/net/wireless/intersil/p54/eeprom.c
+++ b/drivers/net/wireless/intersil/p54/eeprom.c
@@ -867,10 +867,8 @@ int p54_parse_eeprom(struct ieee80211_hw *dev, void
*eeprom, int len)
"test!\n");
err = -ENOMSG;
goto err;
- } else {
- goto good_eeprom;
}
- break;
+ goto good_eeprom;
default:
break;
}
Re: [PATCH] [v5] wireless: Initial driver submission for pureLiFi STA devices
On Mon, 2020-10-19 at 08:47 +0530, Srinivasan Raju wrote:
> This introduces the pureLiFi LiFi driver for LiFi-X, LiFi-XC
> and LiFi-XL USB devices.
Mostly trivial comments:
> diff --git a/drivers/net/wireless/purelifi/chip.c
> b/drivers/net/wireless/purelifi/chip.c
[]
> +int purelifi_chip_set_rate(struct purelifi_chip *chip, u8 rate)
> +{
> + int r;
> + static struct purelifi_chip *chip_p;
Isn't chip_p pointless?
> +
> + if (chip)
> + chip_p = chip;
> + if (!chip_p)
> + return -EINVAL;
> +
chip_p is otherwise unused.
> diff --git a/drivers/net/wireless/purelifi/mac.c
> b/drivers/net/wireless/purelifi/mac.c
[]
> +int purelifi_mac_init_hw(struct ieee80211_hw *hw)
> +{
> + int r;
> + struct purelifi_mac *mac = purelifi_hw_mac(hw);
> + struct purelifi_chip *chip = &mac->chip;
> +
> + r = purelifi_chip_init_hw(chip);
> + if (r) {
> + dev_warn(purelifi_mac_dev(mac), "init hw failed (%d)\n", r);
> + goto out;
> + }
> +
> + dev_dbg(purelifi_mac_dev(mac), "irq_disabled %d\n", irqs_disabled());
> +
> + r = regulatory_hint(hw->wiphy, "CA");
> +out:
> + return r;
> +}
Simpler without the out: label and a direct return
if (r) {
dev_warn(...)
return r;
}
...
return regulator_hint(hw->wiphy, "CA");
}
> +static int download_fpga(struct usb_interface *intf)
> +{
[]
> + if ((le16_to_cpu(udev->descriptor.idVendor) ==
> + PURELIFI_X_VENDOR_ID_0) &&
> + (le16_to_cpu(udev->descriptor.idProduct) ==
> + PURELIFI_X_PRODUCT_ID_0)) {
> + fw_name = "purelifi/li_fi_x/fpga.bit";
> + dev_info(&intf->dev, "bit file for X selected.\n");
> +
> + } else if ((le16_to_cpu(udev->descriptor.idVendor)) ==
> + PURELIFI_XC_VENDOR_ID_0 &&
> +(le16_to_cpu(udev->descriptor.idProduct) ==
> + PURELIFI_XC_PRODUCT_ID_0)) {
> + fw_name = "purelifi/li_fi_x/fpga_xc.bit";
> + dev_info(&intf->dev, "bit filefor XC selected.\n");
Inconsistent dev_info spacing: "file for" vs "filefor"
> + for (fw_data_i = 0; fw_data_i < fw->size;) {
> + int tbuf_idx;
> +
> + if ((fw->size - fw_data_i) < blk_tran_len)
> + blk_tran_len = fw->size - fw_data_i;
> +
> + fw_data = kmalloc(blk_tran_len, GFP_KERNEL);
> +
> + memcpy(fw_data, &fw->data[fw_data_i], blk_tran_len);
> +
> + for (tbuf_idx = 0; tbuf_idx < blk_tran_len; tbuf_idx++) {
> + fw_data[tbuf_idx] =
> + ((fw_data[tbuf_idx] & 128) >> 7) |
> + ((fw_data[tbuf_idx] & 64) >> 5) |
> + ((fw_data[tbuf_idx] & 32) >> 3) |
> + ((fw_data[tbuf_idx] & 16) >> 1) |
> + ((fw_data[tbuf_idx] & 8) << 1) |
> + ((fw_data[tbuf_idx] & 4) << 3) |
> + ((fw_data[tbuf_idx] & 2) << 5) |
> + ((fw_data[tbuf_idx] & 1) << 7);
> + }
pity there isn't an u8_bit_reverse function/mechanism
> +static void pretty_print_mac(struct usb_interface *intf, char *serial_number,
> + unsigned char *hw_address
> + )
> +{
> + unsigned char i;
> +
> + for (i = 0; i < ETH_ALEN; i++)
> + dev_info(&intf->dev, "hw_address[%d]=%x\n", i, hw_address[i]);
I don't think this prettier than %pM
> +}
> +
> +static int upload_mac_and_serial_number(struct usb_interface *intf,
> + unsigned char *hw_address,
> + unsigned char *serial_number)
> +{
[]
> + do {
> + unsigned char buf[8];
> +
> + msleep(200);
> +
> + send_vendor_request(udev, 0x40, buf, sizeof(buf));
> + flash.enabled = buf[0] & 0xFF;
> +
> + if (flash.enabled) {
> + flash.sectors = ((buf[6] & 255) << 24) |
buf is unsigned char[], rather pointless using & 255
> diff --git a/drivers/net/wireless/purelifi/usb.h
> b/drivers/net/wireless/purelifi/usb.h
[]
> +struct station_t {
> + // 7...3|2 | 1 | 0 |
> + // Reserved | Hearbeat | FIFO full | Connected |
heartbeat
Re: [Ocfs2-devel] [RFC] treewide: cleanup unreachable breaks
On Sun, 2020-10-18 at 19:59 +0100, Matthew Wilcox wrote: > On Sat, Oct 17, 2020 at 09:09:28AM -0700, t...@redhat.com wrote: > > clang has a number of useful, new warnings see > > https://urldefense.com/v3/__https://clang.llvm.org/docs/DiagnosticsReference.html__;!!GqivPVa7Brio!Krxz78O3RKcB9JBMVo_F98FupVhj_jxX60ddN6tKGEbv_cnooXc1nnBmchm-e_O9ieGnyQ$ > > > > Please get your IT department to remove that stupidity. If you can't, > please send email from a non-Red Hat email address. I didn't get it this way, neither did lore. It's on your end. https://lore.kernel.org/lkml/20201017160928.12698-1-t...@redhat.com/ > I don't understand why this is a useful warning to fix. Precision in coding style intent and code minimization would be the biggest factors IMO. > What actual problem is caused by the code below? Obviously none.
Re: [Cocci] [RFC] treewide: cleanup unreachable breaks
On Sat, 2020-10-17 at 20:21 +0200, Julia Lawall wrote: > On Sat, 17 Oct 2020, Joe Perches wrote: > > On Sat, 2020-10-17 at 09:09 -0700, t...@redhat.com wrote: > > > From: Tom Rix > > > > > > This is a upcoming change to clean up a new warning treewide. > > > I am wondering if the change could be one mega patch (see below) or > > > normal patch per file about 100 patches or somewhere half way by > > > collecting > > > early acks. > > > > > > clang has a number of useful, new warnings see > > > https://clang.llvm.org/docs/DiagnosticsReference.html > > > > > > This change cleans up -Wunreachable-code-break > > > https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-break > > > for 266 of 485 warnings in this week's linux-next, allyesconfig on x86_64. > > > > Early acks/individual patches by subsystem would be good. > > Better still would be an automated cocci script. > > Coccinelle is not especially good at this, because it is based on control > flow, and a return or goto diverts the control flow away from the break. > A hack to solve the problem is to put an if around the return or goto, but > that gives the break a meaningless file name and line number. I collected > the following list, but it only has 439 results, so fewer than clang. But > maybe there are some files that are not considered by clang in the x86 > allyesconfig configuration. > > Probably checkpatch is the best solution here, since it is not > configuration sensitive and doesn't care about control flow. Likely the clang compiler is the best option here. It might be useful to add -Wunreachable-code-break to W=1 or just always enable it if it isn't already enabled. diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn index 95e4cdb94fe9..3819787579d5 100644 --- a/scripts/Makefile.extrawarn +++ b/scripts/Makefile.extrawarn @@ -32,6 +32,7 @@ KBUILD_CFLAGS += $(call cc-option, -Wunused-but-set-variable) KBUILD_CFLAGS += $(call cc-option, -Wunused-const-variable) KBUILD_CFLAGS += $(call cc-option, -Wpacked-not-aligned) KBUILD_CFLAGS += $(call cc-option, -Wstringop-truncation) +KBUILD_CFLAGS += $(call cc-option, -Wunreachable-code-break) # The following turn off the warnings enabled by -Wextra KBUILD_CFLAGS += -Wno-missing-field-initializers KBUILD_CFLAGS += -Wno-sign-compare (and thank you Tom for pushing this forward) checkpatch can't find instances like: case FOO: if (foo) return 1; else return 2; break; As it doesn't track flow and relies on the number of tabs to be the same for any goto/return and break; checkpatch will warn on: case FOO: ... goto bar; break;
Re: [RFC] treewide: cleanup unreachable breaks
On Sat, 2020-10-17 at 09:09 -0700, t...@redhat.com wrote: > From: Tom Rix > > This is a upcoming change to clean up a new warning treewide. > I am wondering if the change could be one mega patch (see below) or > normal patch per file about 100 patches or somewhere half way by collecting > early acks. > > clang has a number of useful, new warnings see > https://clang.llvm.org/docs/DiagnosticsReference.html > > This change cleans up -Wunreachable-code-break > https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-break > for 266 of 485 warnings in this week's linux-next, allyesconfig on x86_64. Early acks/individual patches by subsystem would be good. Better still would be an automated cocci script. The existing checkpatch test for UNNECESSARY_BREAK has a few too many false positives. >From a script run on next on July 28th: arch/arm/mach-s3c24xx/mach-rx1950.c:266: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/arm/nwfpe/fpa11_cprt.c:38: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/arm/nwfpe/fpa11_cprt.c:41: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:684: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:687: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:690: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:693: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:697: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:700: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:276: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:279: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:282: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:287: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:290: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/rb532/setup.c:76: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/rb532/setup.c:79: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:231: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:234: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:237: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:240: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/net/bpf_jit_comp.c:455: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/platforms/cell/spufs/switch.c:2047: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/platforms/cell/spufs/switch.c:2077: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/sh/boards/mach-landisk/gio.c:111: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/x86/kernel/cpu/mce/core.c:1734: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/x86/kernel/cpu/mce/core.c:1738: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/x86/kernel/cpu/microcode/amd.c:218: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:107: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:132: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:147: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:158: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/ata/libata-scsi.c:3973: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/base/power/main.c:366: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/block/xen-blkback/blkback.c:1272: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/char/ipmi/ipmi_devintf.c:493: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/char/lp.c:625: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/char/mwave/mwavedd.c:406: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/cpufreq/
Re: [PATCH] [v4] wireless: Initial driver submission for pureLiFi STA devices
On Fri, 2020-10-16 at 12:04 +0530, Srinivasan Raju wrote:
> This introduces the pureLiFi LiFi driver for LiFi-X, LiFi-XC
> and LiFi-XL USB devices.
Suggested neatening patch on top of this:
o Use include "usb.h" instead of direct extern
o Prefix purelifi to speed and send_packet_from_data_queue functions
to remove generic global function naming
o Use enum instead of macro defines for MODULATION_RATE_
o Simplify the (mac->chip->)usb->tx. uses with a temporary
o Use continue in loops instead of extra indentation
o Remove unnecessary %hx printf formatting, use %x
---
drivers/net/wireless/purelifi/chip.c | 4 +-
drivers/net/wireless/purelifi/mac.c | 60 +++--
drivers/net/wireless/purelifi/mac.h | 22 -
drivers/net/wireless/purelifi/usb.c | 86
drivers/net/wireless/purelifi/usb.h | 4 +-
5 files changed, 80 insertions(+), 96 deletions(-)
diff --git a/drivers/net/wireless/purelifi/chip.c
b/drivers/net/wireless/purelifi/chip.c
index 9ad2664b7542..cca03697cb06 100644
--- a/drivers/net/wireless/purelifi/chip.c
+++ b/drivers/net/wireless/purelifi/chip.c
@@ -42,12 +42,12 @@ int purelifi_chip_init_hw(struct purelifi_chip *chip)
u8 *addr = purelifi_mac_get_perm_addr(purelifi_chip_to_mac(chip));
struct usb_device *udev = interface_to_usbdev(chip->usb.intf);
- pr_info("purelifi chip %02hx:%02hx v%02hx %02x-%02x-%02x %s\n",
+ pr_info("purelifi chip %02x:%02x v%02x %02x-%02x-%02x %s\n",
le16_to_cpu(udev->descriptor.idVendor),
le16_to_cpu(udev->descriptor.idProduct),
le16_to_cpu(udev->descriptor.bcdDevice),
addr[0], addr[1], addr[2],
- speed(udev->speed));
+ purelifi_speed(udev->speed));
return purelifi_set_beacon_interval(chip, 100, 0, 0);
}
diff --git a/drivers/net/wireless/purelifi/mac.c
b/drivers/net/wireless/purelifi/mac.c
index 6920a1dfd599..09b7c2c0050d 100644
--- a/drivers/net/wireless/purelifi/mac.c
+++ b/drivers/net/wireless/purelifi/mac.c
@@ -10,6 +10,7 @@
#include "chip.h"
#include "mac.h"
+#include "usb.h"
#ifndef IEEE80211_BAND_2GHZ
#define IEEE80211_BAND_2GHZ NL80211_BAND_2GHZ
@@ -332,7 +333,6 @@ static int fill_ctrlset(struct purelifi_mac *mac, struct
sk_buff *skb)
return 0;
}
-extern void send_packet_from_data_queue(struct purelifi_usb *usb);
/**
* purelifi_op_tx - transmits a network frame to the device
*
@@ -366,16 +366,15 @@ static void purelifi_op_tx(struct ieee80211_hw *hw,
u8 dst_mac[ETH_ALEN];
u8 sidx;
bool found = false;
+ struct purelifi_usb_tx *tx = &usb->tx;
memcpy(dst_mac, &skb->data[28], ETH_ALEN);
for (sidx = 0; sidx < MAX_STA_NUM; sidx++) {
- if (usb->tx.station[sidx].flag &
- STATION_CONNECTED_FLAG) {
- if (!memcmp(usb->tx.station[sidx].mac,
- dst_mac, ETH_ALEN)) {
- found = true;
- break;
- }
+ if (!(tx->station[sidx].flag & STATION_CONNECTED_FLAG))
+ continue;
+ if (!memcmp(tx->station[sidx].mac, dst_mac, ETH_ALEN)) {
+ found = true;
+ break;
}
}
@@ -384,13 +383,13 @@ static void purelifi_op_tx(struct ieee80211_hw *hw,
sidx = STA_BROADCAST_INDEX;
/* Stop OS from sending packets, if the queue is half full */
- if (skb_queue_len(&usb->tx.station[sidx].data_list) > 60)
- block_queue(usb, usb->tx.station[sidx].mac, true);
+ if (skb_queue_len(&tx->station[sidx].data_list) > 60)
+ block_queue(usb, tx->station[sidx].mac, true);
/* Schedule packet for transmission if queue is not full */
- if (skb_queue_len(&usb->tx.station[sidx].data_list) < 256) {
- skb_queue_tail(&usb->tx.station[sidx].data_list, skb);
- send_packet_from_data_queue(usb);
+ if (skb_queue_len(&tx->station[sidx].data_list) < 256) {
+ skb_queue_tail(&tx->station[sidx].data_list, skb);
+ purelifi_send_packet_from_data_queue(usb);
} else {
dev_kfree_skb(skb);
}
@@ -487,6 +486,7 @@ int purelifi_mac_rx(struct ieee80211_hw *hw, const u8
*buffer,
static unsigned short int min_exp_seq_nmb;
u32 crc_error_cnt_low, crc_error_cnt_high;
int sidx;
+ struct purelifi_usb_tx *tx;
/* Packet blockade during disabled interface. */
if (!mac->vif)
@@ -530,33 +530,25 @@ i
Re: [PATCH] [PATCH] [v3] wireless: Initial driver submission for pureLiFi STA devices
On Wed, 2020-10-14 at 11:49 +0530, Srinivasan Raju wrote:
> This introduces the pureLiFi LiFi driver for LiFi-X, LiFi-XC
> and LiFi-XL USB devices.
trivia: netdev_ might be better than dev_.
> diff --git a/drivers/net/wireless/purelifi/chip.c
> b/drivers/net/wireless/purelifi/chip.c
[]
> +int purelifi_chip_switch_radio(struct purelifi_chip *chip, u32 value)
> +{
> + int r;
> +
> + r = usb_write_req((const u8 *)&value, sizeof(value), USB_REQ_POWER_WR);
> + if (r)
> + dev_err(purelifi_chip_dev(chip), "%s r=%d", __func__, r);
missing '\n' termination.
[]
> > +int purelifi_chip_set_rate(struct purelifi_chip *chip, u8 rate)
> +{
> + int r;
> + static struct purelifi_chip *chip_p;
> +
> + if (chip)
> + chip_p = chip;
> + if (!chip_p)
> + return -EINVAL;
> +
> + r = usb_write_req((const u8 *)&rate, sizeof(rate), USB_REQ_RATE_WR);
> + if (r)
> + dev_err(purelifi_chip_dev(chip), "set rate failed r=%d", r);
same missing newline, etc
It might also be better to use a consistent error message like
dev_err(purelifi_chip_dev(chip), "%s: failed, r=%d\n", r);
for both.
> diff --git a/drivers/net/wireless/purelifi/mac.c
> b/drivers/net/wireless/purelifi/mac.c
[]
> +static const struct ieee80211_rate purelifi_rates[] = {
> + { .bitrate = 10,
> + .hw_value = PURELIFI_CCK_RATE_1M, },
[]
> + { .bitrate = 60,
> + .hw_value = PURELIFI_OFDM_RATE_6M,
> + .flags = 0 },
Seems odd to set .flags to 0 for only some of the entries.
Perhaps more readable to always leave it off if unset.
> + { .bitrate = 90,
> + .hw_value = PURELIFI_OFDM_RATE_9M,
> + .flags = 0 },
> + { .bitrate = 120,
> + .hw_value = PURELIFI_OFDM_RATE_12M,
> + .flags = 0 },
> + { .bitrate = 180,
> + .hw_value = PURELIFI_OFDM_RATE_18M,
> + .flags = 0 },
> + { .bitrate = 240,
> + .hw_value = PURELIFI_OFDM_RATE_24M,
> + .flags = 0 },
> + { .bitrate = 360,
> + .hw_value = PURELIFI_OFDM_RATE_36M,
> + .flags = 0 },
> + { .bitrate = 480,
> + .hw_value = PURELIFI_OFDM_RATE_48M,
> + .flags = 0 },
> + { .bitrate = 540,
> + .hw_value = PURELIFI_OFDM_RATE_54M,
> + .flags = 0 },
> +};
[]
> +int purelifi_mac_init_hw(struct ieee80211_hw *hw)
> +{
> + int r;
> + struct purelifi_mac *mac = purelifi_hw_mac(hw);
> + struct purelifi_chip *chip = &mac->chip;
> +
> + r = purelifi_chip_init_hw(chip);
> + if (r) {
> + dev_warn(purelifi_mac_dev(mac), "init hw failed. (%d)\n", r);
> + goto out;
> + }
> +
> + dev_dbg(purelifi_mac_dev(mac), "irq_disabled %d", irqs_disabled());
missing newline
> +static int filter_ack(struct ieee80211_hw *hw, struct ieee80211_hdr *rx_hdr,
> + struct ieee80211_rx_status *stats)
> +{
> + struct purelifi_mac *mac = purelifi_hw_mac(hw);
> + struct sk_buff *skb;
> + struct sk_buff_head *q;
> + unsigned long flags;
> + int found = 0;
> + int i, position = 0;
> +
> + if (!ieee80211_is_ack(rx_hdr->frame_control))
> + return 0;
> + dev_info(purelifi_mac_dev(mac), "%s::ACK Received", __func__);
missing newline
> +static int purelifi_op_add_interface(struct ieee80211_hw *hw,
> + struct ieee80211_vif *vif)
> +{
> + struct purelifi_mac *mac = purelifi_hw_mac(hw);
> + static const char * const iftype80211[] = {
> + [NL80211_IFTYPE_STATION]= "Station",
> + [NL80211_IFTYPE_ADHOC] = "Adhoc"
> + };
> +
> + if (mac->type != NL80211_IFTYPE_UNSPECIFIED)
> + return -EOPNOTSUPP;
> +
> + if (vif->type == NL80211_IFTYPE_ADHOC ||
> + vif->type == NL80211_IFTYPE_STATION) {
> + dev_info(purelifi_mac_dev(mac), "%s %s\n", __func__,
> + iftype80211[vif->type]);
> + mac->type = vif->type;
> + mac->vif = vif;
> + } else {
> + dev_info(purelifi_mac_dev(mac), "unsupported iftype");
etc...
iwlwifi: spaces in procfs filenames ?
commit 64fa3aff89785b5a924ce3934f6595c35b4dffee
Author: Sharon Dvir
Date: Wed Aug 17 15:35:09 2016 +0300
iwlwifi: pcie: give a meaningful name to interrupt request
perhaps unintentionally for file:
drivers/net/wireless/intel/iwlwifi/pcie/internal.h
in function static inline const char *queue_name
creates spaces in procfs filenames.
drivers/net/wireless/intel/iwlwifi/pcie/internal.h:static inline const char
*queue_name(struct device *dev,
drivers/net/wireless/intel/iwlwifi/pcie/internal.h-
struct iwl_trans_pcie *trans_p, int i)
drivers/net/wireless/intel/iwlwifi/pcie/internal.h-{
drivers/net/wireless/intel/iwlwifi/pcie/internal.h- if
(trans_p->shared_vec_mask) {
drivers/net/wireless/intel/iwlwifi/pcie/internal.h- int vec =
trans_p->shared_vec_mask &
drivers/net/wireless/intel/iwlwifi/pcie/internal.h-
IWL_SHARED_IRQ_FIRST_RSS ? 1 : 0;
drivers/net/wireless/intel/iwlwifi/pcie/internal.h-
drivers/net/wireless/intel/iwlwifi/pcie/internal.h- if (i == 0)
drivers/net/wireless/intel/iwlwifi/pcie/internal.h- return
DRV_NAME ": shared IRQ";
drivers/net/wireless/intel/iwlwifi/pcie/internal.h-
drivers/net/wireless/intel/iwlwifi/pcie/internal.h- return
devm_kasprintf(dev, GFP_KERNEL,
drivers/net/wireless/intel/iwlwifi/pcie/internal.h-
DRV_NAME ": queue %d", i + vec);
drivers/net/wireless/intel/iwlwifi/pcie/internal.h- }
drivers/net/wireless/intel/iwlwifi/pcie/internal.h- if (i == 0)
drivers/net/wireless/intel/iwlwifi/pcie/internal.h- return DRV_NAME
": default queue";
drivers/net/wireless/intel/iwlwifi/pcie/internal.h-
drivers/net/wireless/intel/iwlwifi/pcie/internal.h- if (i ==
trans_p->alloc_vecs - 1)
drivers/net/wireless/intel/iwlwifi/pcie/internal.h- return DRV_NAME
": exception";
drivers/net/wireless/intel/iwlwifi/pcie/internal.h-
drivers/net/wireless/intel/iwlwifi/pcie/internal.h- return
devm_kasprintf(dev, GFP_KERNEL,
drivers/net/wireless/intel/iwlwifi/pcie/internal.h-
DRV_NAME ": queue %d", i);
drivers/net/wireless/intel/iwlwifi/pcie/internal.h-}
# find /proc/ | grep " "
/proc/irq/130/iwlwifi: default queue
/proc/irq/131/iwlwifi: queue 1
/proc/irq/132/iwlwifi: queue 2
/proc/irq/133/iwlwifi: queue 3
/proc/irq/134/iwlwifi: queue 4
/proc/irq/135/iwlwifi: exception
Can these names be changed back or collapsed
to avoid the space use in procfs?
Re: [PATCH AUTOSEL 5.8 18/24] net: usb: rtl8150: set random MAC address when set_ethernet_addr() fails
On Mon, 2020-10-12 at 15:02 -0400, Sasha Levin wrote:
> From: Anant Thazhemadam
>
> [ Upstream commit f45a4248ea4cc13ed50618ff066849f9587226b2 ]
>
> When get_registers() fails in set_ethernet_addr(),the uninitialized
> value of node_id gets copied over as the address.
> So, check the return value of get_registers().
>
> If get_registers() executed successfully (i.e., it returns
> sizeof(node_id)), copy over the MAC address using ether_addr_copy()
> (instead of using memcpy()).
>
> Else, if get_registers() failed instead, a randomly generated MAC
> address is set as the MAC address instead.
This autosel is premature.
This patch always sets a random MAC.
See the follow on patch: https://lkml.org/lkml/2020/10/11/131
To my knowledge, this follow-ob has yet to be applied:
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
[]
> @@ -274,12 +274,20 @@ static int write_mii_word(rtl8150_t * dev, u8 phy, __u8
> indx, u16 reg)
> return 1;
> }
>
> -static inline void set_ethernet_addr(rtl8150_t * dev)
> +static void set_ethernet_addr(rtl8150_t *dev)
> {
> - u8 node_id[6];
> + u8 node_id[ETH_ALEN];
> + int ret;
> +
> + ret = get_registers(dev, IDR, sizeof(node_id), node_id);
>
> - get_registers(dev, IDR, sizeof(node_id), node_id);
> - memcpy(dev->netdev->dev_addr, node_id, sizeof(node_id));
> + if (ret == sizeof(node_id)) {
So this needs to use
if (!ret) {
or
if (ret < 0)
and reversed code blocks
> + ether_addr_copy(dev->netdev->dev_addr, node_id);
> + } else {
> + eth_hw_addr_random(dev->netdev);
> + netdev_notice(dev->netdev, "Assigned a random MAC address:
> %pM\n",
> + dev->netdev->dev_addr);
> + }
> }
>
> static int rtl8150_set_mac_address(struct net_device *netdev, void *p)
Re: [PATCH v2] net: usb: rtl8150: don't incorrectly assign random MAC addresses
On Sun, 2020-10-11 at 22:31 +0300, Petko Manolov wrote: > On 20-10-11 11:33:00, Joe Perches wrote: > > On Sun, 2020-10-11 at 10:59 -0700, Jakub Kicinski wrote: > > > On Sun, 11 Oct 2020 23:00:30 +0530 Anant Thazhemadam wrote: > > > > In set_ethernet_addr(), if get_registers() succeeds, the ethernet > > > > address > > > > that was read must be copied over. Otherwise, a random ethernet address > > > > must be assigned. > > > > > > > > get_registers() returns 0 if successful, and negative error number > > > > otherwise. However, in set_ethernet_addr(), this return value is > > > > incorrectly checked. > > > > > > > > Since this return value will never be equal to sizeof(node_id), a > > > > random MAC address will always be generated and assigned to the > > > > device; even in cases when get_registers() is successful. > > > > > > > > Correctly modifying the condition that checks if get_registers() was > > > > successful or not fixes this problem, and copies the ethernet address > > > > appropriately. > > > > There are many unchecked uses of set_registers and get_registers > > in this file. > > > > If failures are really expected, then it might be better to fix > > them up too. > > Checking the return value of each get/set_registers() is going to be a PITA > and > not very helpful. Doing so when setting the MAC address _does_ make sense as > in > that case it is not a hard error. > > In almost all other occasions if usb_control_msg_send/recv() return an error > i'd > rather dump an error message (from within get/set_registers()) and let the > user > decide whether to get rid of this adapter or start debugging it. Your code, your choices... Consider using _once or _ratelimited output too.
Re: [PATCH v2] net: usb: rtl8150: don't incorrectly assign random MAC addresses
On Sun, 2020-10-11 at 10:59 -0700, Jakub Kicinski wrote: > On Sun, 11 Oct 2020 23:00:30 +0530 Anant Thazhemadam wrote: > > In set_ethernet_addr(), if get_registers() succeeds, the ethernet address > > that was read must be copied over. Otherwise, a random ethernet address > > must be assigned. > > > > get_registers() returns 0 if successful, and negative error number > > otherwise. However, in set_ethernet_addr(), this return value is > > incorrectly checked. > > > > Since this return value will never be equal to sizeof(node_id), a > > random MAC address will always be generated and assigned to the > > device; even in cases when get_registers() is successful. > > > > Correctly modifying the condition that checks if get_registers() was > > successful or not fixes this problem, and copies the ethernet address > > appropriately. There are many unchecked uses of set_registers and get_registers in this file. If failures are really expected, then it might be better to fix them up too. $ git grep -w '[gs]et_registers' drivers/net/usb/rtl8150.c drivers/net/usb/rtl8150.c:static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data) drivers/net/usb/rtl8150.c:static int set_registers(rtl8150_t * dev, u16 indx, u16 size, const void *data) drivers/net/usb/rtl8150.c: set_registers(dev, PHYADD, sizeof(data), data); drivers/net/usb/rtl8150.c: set_registers(dev, PHYCNT, 1, &tmp); drivers/net/usb/rtl8150.c: get_registers(dev, PHYCNT, 1, data); drivers/net/usb/rtl8150.c: get_registers(dev, PHYDAT, 2, data); drivers/net/usb/rtl8150.c: set_registers(dev, PHYADD, sizeof(data), data); drivers/net/usb/rtl8150.c: set_registers(dev, PHYCNT, 1, &tmp); drivers/net/usb/rtl8150.c: get_registers(dev, PHYCNT, 1, data); drivers/net/usb/rtl8150.c: ret = get_registers(dev, IDR, sizeof(node_id), node_id); drivers/net/usb/rtl8150.c: set_registers(dev, IDR, netdev->addr_len, netdev->dev_addr); drivers/net/usb/rtl8150.c: get_registers(dev, CR, 1, &cr); drivers/net/usb/rtl8150.c: set_registers(dev, CR, 1, &cr); drivers/net/usb/rtl8150.c: set_registers(dev, IDR_EEPROM + (i * 2), 2, drivers/net/usb/rtl8150.c: set_registers(dev, CR, 1, &cr); drivers/net/usb/rtl8150.c: set_registers(dev, CR, 1, &data); drivers/net/usb/rtl8150.c: get_registers(dev, CR, 1, &data); drivers/net/usb/rtl8150.c: set_registers(dev, RCR, 1, &rcr); drivers/net/usb/rtl8150.c: set_registers(dev, TCR, 1, &tcr); drivers/net/usb/rtl8150.c: set_registers(dev, CR, 1, &cr); drivers/net/usb/rtl8150.c: get_registers(dev, MSR, 1, &msr); drivers/net/usb/rtl8150.c: get_registers(dev, CR, 1, &cr); drivers/net/usb/rtl8150.c: set_registers(dev, CR, 1, &cr); drivers/net/usb/rtl8150.c: get_registers(dev, CSCR, 2, &tmp); drivers/net/usb/rtl8150.c: set_registers(dev, IDR, 6, netdev->dev_addr); drivers/net/usb/rtl8150.c: get_registers(dev, BMCR, 2, &bmcr); drivers/net/usb/rtl8150.c: get_registers(dev, ANLP, 2, &lpa);
Re: [PATCH] rtlwifi: rtl8192se: remove duplicated legacy_httxpowerdiff
On Tue, 2020-10-06 at 11:59 +0800, Chris Chiu wrote: > From: Chris Chiu > > The legacy_httxpowerdiff in rtl8192se is pretty much the same as > the legacy_ht_txpowerdiff for other chips. Use the same name to > keep the consistency. > > Signed-off-by: Chris Chiu > --- > drivers/net/wireless/realtek/rtlwifi/rtl8192se/hw.c | 2 +- > drivers/net/wireless/realtek/rtlwifi/rtl8192se/rf.c | 2 +- > drivers/net/wireless/realtek/rtlwifi/wifi.h | 1 - > 3 files changed, 2 insertions(+), 3 deletions(-) Then can't all the struct definitions that include legacy_ht_txpowerdiff other than wifi.h delete it too? $ git grep -P -n '\blegacy_ht_?txpower' -- '*.h' drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.h:162: u8 legacy_ht_txpowerdiff; drivers/net/wireless/realtek/rtlwifi/rtl8192c/phy_common.h:155: u8 legacy_ht_txpowerdiff; drivers/net/wireless/realtek/rtlwifi/rtl8723ae/phy.h:140: u8 legacy_ht_txpowerdiff; drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.h:170: u8 legacy_ht_txpowerdiff; drivers/net/wireless/realtek/rtlwifi/wifi.h:1969: u8 legacy_httxpowerdiff;/* Legacy to HT rate power diff */ drivers/net/wireless/realtek/rtlwifi/wifi.h:1980: u8 legacy_ht_txpowerdiff; /*Legacy to HT rate power diff */
Re: [PATCH v3] net: usb: rtl8150: set random MAC address when set_ethernet_addr() fails
On Sun, 2020-10-04 at 02:49 +0530, Anant Thazhemadam wrote:
> When get_registers() fails, in set_ethernet_addr(),the uninitialized
> value of node_id gets copied as the address. This can be considered as
> set_ethernet_addr() itself failing.
[]
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
[]
> @@ -909,7 +914,10 @@ static int rtl8150_probe(struct usb_interface *intf,
> goto out1;
> }
> fill_skb_pool(dev);
> - set_ethernet_addr(dev);
> + if (!set_ethernet_addr(dev)) {
> + dev_err(&intf->dev, "assigining a random MAC address\n");
> + eth_hw_addr_random(dev->netdev);
4 things:
o Typo for assigning
o Reverse the assignment and message to show the new random MAC
o This should use netdev_
o Is this better as error or notification?
if (!set_ethernet_addr(dev)) {
eth_hw_addr_random(dev->netdev);
netdev_notice(dev->netdev, "Assigned a random MAC: %pM\n",
dev->netdev->dev_addr);
}
Re: [Linux-kernel-mentees][PATCH v2] net: usb: rtl8150: prevent set_ethernet_addr from setting uninit address
On Thu, 2020-10-01 at 13:02 +0530, Anant Thazhemadam wrote:
> When get_registers() fails (which happens when usb_control_msg() fails)
> in set_ethernet_addr(), the uninitialized value of node_id gets copied
> as the address.
unrelated trivia:
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
[]
> @@ -274,12 +274,17 @@ static int write_mii_word(rtl8150_t * dev, u8 phy, __u8
> indx, u16 reg)
> return 1;
> }
>
> -static inline void set_ethernet_addr(rtl8150_t * dev)
> +static bool set_ethernet_addr(rtl8150_t *dev)
> {
> u8 node_id[6];
This might be better as:
u8 node_id[ETH_ALEN];
> + int ret;
>
> - get_registers(dev, IDR, sizeof(node_id), node_id);
> - memcpy(dev->netdev->dev_addr, node_id, sizeof(node_id));
> + ret = get_registers(dev, IDR, sizeof(node_id), node_id);
> + if (ret == sizeof(node_id)) {
> + memcpy(dev->netdev->dev_addr, node_id, sizeof(node_id));
and
ether_addr_copy(dev->netdev->dev_addr, node_id);
Re: [PATCH] [v2] wireless: Initial driver submission for pureLiFi devices
On Mon, 2020-09-28 at 15:49 +0530, Srinivasan Raju wrote:
> This introduces the pureLiFi LiFi driver for LiFi-X, LiFi-XC
> and LiFi-XL USB devices, which provide lightweight, highspeed secure and
> fully networked wireless communications via light.
trivial notes:
> diff --git a/drivers/net/wireless/purelifi/chip.c
> b/drivers/net/wireless/purelifi/chip.c
[]
> +static void print_chip_info(struct purelifi_chip *chip)
> +{
> + u8 *addr = purelifi_mac_get_perm_addr(purelifi_chip_to_mac(chip));
> + struct usb_device *udev = interface_to_usbdev(chip->usb.intf);
> +
> + pr_info("purelifi chip %04hx:%04hx v%04hx %02x-%02x-%02x %s\n",
You don't need to use %04hx for u16's
any more than you need to use %02hhx for u8's.
pr_info("purelifi chip %04x:%04x v%04x %02x-%02x-%02x %s\n",
> + le16_to_cpu(udev->descriptor.idVendor),
> + le16_to_cpu(udev->descriptor.idProduct),
> + le16_to_cpu(udev->descriptor.bcdDevice),
> + addr[0], addr[1], addr[2],
> + speed(udev->speed));
> +}
>
> +static inline void purelifi_mc_add_addr(struct purelifi_mc_hash *hash,
> + u8 *addr
> + )
Can you use normal close parenthesis locations?
> diff --git a/drivers/net/wireless/purelifi/def.h
> b/drivers/net/wireless/purelifi/def.h
> new file mode 100644
> index ..295dfb45b568
> --- /dev/null
> +++ b/drivers/net/wireless/purelifi/def.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _PURELIFI_DEF_H
> +#define _PURELIFI_DEF_H
> +
> +#include
> +#include
> +#include
> +
> +typedef u16 __nocast purelifi_addr_t;
> +
> +#define dev_printk_f(level, dev, fmt, args...) \
> + dev_printk(level, dev, "%s() " fmt, __func__, ##args)
If you _really_w want __func__ output you could use
#define dev_fmt "%s(): " fmt, __func__
> +
> +#ifdef DEBUG
> +# define dev_dbg_f(dev, fmt, args...) \
> + dev_printk_f(KERN_DEBUG, dev, fmt, ## args)
> +# define dev_dbg_f_limit(dev, fmt, args...) do { \
> + if (net_ratelimit()) \
> + dev_printk_f(KERN_DEBUG, dev, fmt, ## args); \
> +} while (0)
> +# define dev_dbg_f_cond(dev, cond, fmt, args...) ({ \
> + bool __cond = !!(cond); \
> + if (unlikely(__cond)) \
> + dev_printk_f(KERN_DEBUG, dev, fmt, ## args); \
> +})
> +#else
> +# define dev_dbg_f(dev, fmt, args...) (void)(dev)
no_printk
> +# define dev_dbg_f_limit(dev, fmt, args...) (void)(dev)
> +# define dev_dbg_f_cond(dev, cond, fmt, args...) (void)(dev)
> +#endif /* DEBUG */
>
[]
> +++ b/drivers/net/wireless/purelifi/log.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _PURELIFI_LOG
> +#define _PURELIFI_LOG
> +
> +#ifdef PL_DEBUG
> +#define pl_dev_info(dev, fmt, arg...) dev_info(dev, fmt, ##arg)
> +#define pl_dev_warn(dev, fmt, arg...) dev_warn(dev, fmt, ##arg)
> +#define pl_dev_err(dev, fmt, arg...) dev_err(dev, fmt, ##arg)
Seems completely pointless.
Debugging output should be at KERN_DEBUG
> +#else
> +#define pl_dev_info(dev, fmt, arg...) (void)(dev)
> +#define pl_dev_warn(dev, fmt, arg...) (void)(dev)
> +#define pl_dev_err(dev, fmt, arg...) (void)(dev)
> +#endif
> +#endif
> diff --git a/drivers/net/wireless/purelifi/mac.c
> b/drivers/net/wireless/purelifi/mac.c
[]
> +int purelifi_mac_init_hw(struct ieee80211_hw *hw)
> +{
> + int r;
> + struct purelifi_mac *mac = purelifi_hw_mac(hw);
> + struct purelifi_chip *chip = &mac->chip;
> +
> + r = purelifi_chip_init_hw(chip);
> + if (r) {
> + pl_dev_warn(purelifi_mac_dev(mac), "%s::purelifi_chip_init_hw
> failed. (%d)\n",
> + __func__, r);
Odd double colon with odd spacing too.
> + goto out;
> + }
> + PURELIFI_ASSERT(!irqs_disabled());
> +
> + r = regulatory_hint(hw->wiphy, "CA");
> +out:
> + return r;
> +}
> +
> +void purelifi_mac_release(struct purelifi_mac *mac)
> +{
> + purelifi_chip_release(&mac->chip);
> + lockdep_assert_held(&mac->lock);
> +}
> +
> +int purelifi_op_start(struct ieee80211_hw *hw)
> +{
> + regulatory_hint(hw->wiphy, "EU");
> + purelifi_hw_mac(hw)->chip.usb.initialized = 1;
> + return 0;
> +}
> +
> +void purelifi_op_stop(struct ieee80211_hw *hw)
> +{
> + struct purelifi_mac *mac = purelifi_hw_mac(hw);
> + struct purelifi_chip *chip = &mac->chip;
> + struct sk_buff *skb;
> + struct sk_buff_head *ack_wait_queue = &mac->ack_wait_queue;
> +
> + pl_dev_info(purelifi_mac_dev(mac), "%s", __func__);
Unnecessary as ftrace works well
[]
> +static int fill_ctrlset(struct purelifi_mac *mac, struct sk_buff *skb)
> +{
> + unsigned int frag_len = skb->len;
> + unsigned int tmp;
> + struct purelifi_ctrlset *cs;
> +
> + if (skb_headroom(skb) >= sizeof(struct purelifi_ctrlset)) {
> + cs = (struct purelifi_ctrlset *)skb_push(skb,
> + sizeof(stru
Re: [patch 00/35] net: in_interrupt() cleanup and fixes
On Sun, 2020-09-27 at 21:48 +0200, Thomas Gleixner wrote: > Folks, > > in the discussion about preempt count consistency accross kernel > configurations: > > https://lore.kernel.org/r/20200914204209.256266...@linutronix.de/ > > Linus clearly requested that code in drivers and libraries which changes > behaviour based on execution context should either be split up so that > e.g. task context invocations and BH invocations have different interfaces > or if that's not possible the context information has to be provided by the > caller which knows in which context it is executing. > > This includes conditional locking, allocation mode (GFP_*) decisions and > avoidance of code paths which might sleep. Are these patches intended to be applied to Linus' tree before v5.9 is released? This patchset will cause conflicts against -next. For instance, in patch 34, RT_TRACE has already been removed in -next.
[PATCH] rtlwifi: Use ffs in _phy_calculate_bit_shift
Remove the loop and use the generic ffs instead.
Signed-off-by: Joe Perches
---
Just saw one by happenstance, might as well fix them all.
drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c | 18 ++
.../net/wireless/realtek/rtlwifi/rtl8192c/phy_common.c | 8 ++--
drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c | 9 ++---
drivers/net/wireless/realtek/rtlwifi/rtl8192ee/phy.c | 8 ++--
drivers/net/wireless/realtek/rtlwifi/rtl8192se/phy.c | 9 ++---
.../wireless/realtek/rtlwifi/rtl8723com/phy_common.c | 8 ++--
drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c | 18 ++
7 files changed, 22 insertions(+), 56 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c
b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c
index 38d4432767e8..9be032e8ec95 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c
@@ -16,7 +16,12 @@ static u32 _rtl88e_phy_rf_serial_read(struct ieee80211_hw
*hw,
static void _rtl88e_phy_rf_serial_write(struct ieee80211_hw *hw,
enum radio_path rfpath, u32 offset,
u32 data);
-static u32 _rtl88e_phy_calculate_bit_shift(u32 bitmask);
+static u32 _rtl88e_phy_calculate_bit_shift(u32 bitmask)
+{
+ u32 i = ffs(bitmask);
+
+ return i ? i - 1 : 32;
+}
static bool _rtl88e_phy_bb8188e_config_parafile(struct ieee80211_hw *hw);
static bool _rtl88e_phy_config_mac_with_headerfile(struct ieee80211_hw *hw);
static bool phy_config_bb_with_headerfile(struct ieee80211_hw *hw,
@@ -208,17 +213,6 @@ static void _rtl88e_phy_rf_serial_write(struct
ieee80211_hw *hw,
rfpath, pphyreg->rf3wire_offset, data_and_addr);
}
-static u32 _rtl88e_phy_calculate_bit_shift(u32 bitmask)
-{
- u32 i;
-
- for (i = 0; i <= 31; i++) {
- if (((bitmask >> i) & 0x1) == 1)
- break;
- }
- return i;
-}
-
bool rtl88e_phy_mac_config(struct ieee80211_hw *hw)
{
struct rtl_priv *rtlpriv = rtl_priv(hw);
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192c/phy_common.c
b/drivers/net/wireless/realtek/rtlwifi/rtl8192c/phy_common.c
index fc6c81291cf5..238b6e010769 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192c/phy_common.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192c/phy_common.c
@@ -145,13 +145,9 @@ EXPORT_SYMBOL(_rtl92c_phy_rf_serial_write);
u32 _rtl92c_phy_calculate_bit_shift(u32 bitmask)
{
- u32 i;
+ u32 i = ffs(bitmask);
- for (i = 0; i <= 31; i++) {
- if (((bitmask >> i) & 0x1) == 1)
- break;
- }
- return i;
+ return i ? i - 1 : 32;
}
EXPORT_SYMBOL(_rtl92c_phy_calculate_bit_shift);
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c
b/drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c
index 87804325928a..e34d33e73e52 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c
@@ -162,14 +162,9 @@ static u32 targetchnl_2g[TARGET_CHNL_NUM_2G] = {
static u32 _rtl92d_phy_calculate_bit_shift(u32 bitmask)
{
- u32 i;
-
- for (i = 0; i <= 31; i++) {
- if (((bitmask >> i) & 0x1) == 1)
- break;
- }
+ u32 i = ffs(bitmask);
- return i;
+ return i ? i - 1 : 32;
}
u32 rtl92d_phy_query_bb_reg(struct ieee80211_hw *hw, u32 regaddr, u32 bitmask)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/phy.c
b/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/phy.c
index f107a30a96f0..cc0bcaf13e96 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/phy.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/phy.c
@@ -203,13 +203,9 @@ static void _rtl92ee_phy_rf_serial_write(struct
ieee80211_hw *hw,
static u32 _rtl92ee_phy_calculate_bit_shift(u32 bitmask)
{
- u32 i;
+ u32 i = ffs(bitmask);
- for (i = 0; i <= 31; i++) {
- if (((bitmask >> i) & 0x1) == 1)
- break;
- }
- return i;
+ return i ? i - 1 : 32;
}
bool rtl92ee_phy_mac_config(struct ieee80211_hw *hw)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192se/phy.c
b/drivers/net/wireless/realtek/rtlwifi/rtl8192se/phy.c
index 3d482b8675e2..63283d9e7485 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192se/phy.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192se/phy.c
@@ -16,14 +16,9 @@
static u32 _rtl92s_phy_calculate_bit_shift(u32 bitmask)
{
- u32 i;
-
- for (i = 0; i <= 31; i++) {
- if (((bitmask >> i) & 0x1) == 1)
- break;
- }
+ u32 i = ffs(bitmask);
- return i;
+ return i ? i - 1 : 32;
}
u32 rtl92s_phy_query_bb_reg(struct ieee80211
Re: [PATCH net-next v5 1/6] lib8390: Fix coding style issues and remove version printing
On Mon, 2020-09-14 at 23:01 +0200, Armin Wolf wrote:
> Fix various checkpatch warnings.
>
> Remove version printing so modules including lib8390 do not
> have to provide a global version string for successful
> compilation.
This 8390 code is for a _really_ old driver.
It doesn't seem likely these changes are all that useful.
(and I say that as someone that's done a lot of drive-by
cleaning to this code, mostly treewide though)
> diff --git a/drivers/net/ethernet/8390/lib8390.c
> b/drivers/net/ethernet/8390/lib8390.c
[]
> @@ -308,25 +301,24 @@ static netdev_tx_t __ei_start_xmit(struct sk_buff *skb,
> char *data = skb->data;
>
> if (skb->len < ETH_ZLEN) {
> - memset(buf, 0, ETH_ZLEN); /* more efficient than doing
> just the needed bits */
> + /* More efficient than doing just the needed bits */
> + memset(buf, 0, ETH_ZLEN);
> memcpy(buf, data, skb->len);
Even with the comment, this bit seems less than optimal.
Maybe not overwriting the already zeroed content is better?
At least it's fewer memory accesses.
memcpy(buf, data, skb->len);
memset(&buf[skb->len], 0, ETH_ZLEN - skb->len);
Re: [PATCH net-next v5 2/6] lib8390: Replace pr_cont() with SMP-safe construct
On Mon, 2020-09-14 at 23:01 +0200, Armin Wolf wrote:
> Replace pr_cont() with SMP-safe construct.
>
> Signed-off-by: Armin Wolf
> ---
> drivers/net/ethernet/8390/lib8390.c | 31 +++--
> 1 file changed, 12 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/ethernet/8390/lib8390.c
> b/drivers/net/ethernet/8390/lib8390.c
> index 3a2b1e33a47a..e8a323352c40 100644
> --- a/drivers/net/ethernet/8390/lib8390.c
> +++ b/drivers/net/ethernet/8390/lib8390.c
> @@ -518,25 +518,18 @@ static void ei_tx_err(struct net_device *dev)
> {
> unsigned long e8390_base = dev->base_addr;
> /* ei_local is used on some platforms via the EI_SHIFT macro */
> - struct ei_device *ei_local __maybe_unused = netdev_priv(dev);
> - unsigned char txsr = ei_inb_p(e8390_base+EN0_TSR);
> - unsigned char tx_was_aborted = txsr & (ENTSR_ABT+ENTSR_FU);
> -
> -#ifdef VERBOSE_ERROR_DUMP
> - netdev_dbg(dev, "transmitter error (%#2x):", txsr);
> - if (txsr & ENTSR_ABT)
> - pr_cont(" excess-collisions ");
> - if (txsr & ENTSR_ND)
> - pr_cont(" non-deferral ");
> - if (txsr & ENTSR_CRS)
> - pr_cont(" lost-carrier ");
> - if (txsr & ENTSR_FU)
> - pr_cont(" FIFO-underrun ");
> - if (txsr & ENTSR_CDH)
> - pr_cont(" lost-heartbeat ");
> - pr_cont("\n");
> -#endif
> -
> + struct ei_device *ei_local = netdev_priv(dev);
> + unsigned char txsr = ei_inb_p(e8390_base + EN0_TSR);
> + unsigned char tx_was_aborted = txsr & (ENTSR_ABT + ENTSR_FU);
> +
> + if (netif_msg_tx_err(ei_local)) {
> + netdev_err(dev, "Transmitter error %#2x ( %s%s%s%s%s)", txsr,
> +(txsr & ENTSR_ABT) ? "excess-collisions " : "",
> +(txsr & ENTSR_ND) ? "non-deferral " : "",
> +(txsr & ENTSR_CRS) ? "lost-carrier " : "",
> +(txsr & ENTSR_FU) ? "FIFO-underrun " : "",
> +(txsr & ENTSR_CDH) ? "lost-heartbeat " : "");
> + }
Still should use a terminating '\n' and likely
this might be better as:
netif_dbg(ei_local, tx_err, dev, "Transmitter error ...\n",
etc...);
Re: [PATCH net-next] octeontx2-af: Constify npc_kpu_profile_{action,cam}
On Sat, 2020-09-12 at 00:00 +0200, Rikard Falkeborn wrote: > These are never modified, so constify them to allow the compiler to > place them in read-only memory. This moves about 25kB to read-only > memory as seen by the output of the size command. Nice. Did you find this by tool or inspection?
Re: [trivial PATCH] treewide: Convert switch/case fallthrough; to break;
On Thu, 2020-09-10 at 15:21 +0100, Robin Murphy wrote:
> On 2020-09-09 21:06, Joe Perches wrote:
> > fallthrough to a separate case/default label break; isn't very readable.
> >
> > Convert pseudo-keyword fallthrough; statements to a simple break; when
> > the next label is case or default and the only statement in the next
> > label block is break;
> >
> > Found using:
> >
> > $ grep-2.5.4 -rP --include=*.[ch] -n
> > "fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
> >
> > Miscellanea:
> >
> > o Move or coalesce a couple label blocks above a default: block.
> >
> > Signed-off-by: Joe Perches
> > ---
> >
> > Compiled allyesconfig x86-64 only.
> > A few files for other arches were not compiled.
> >
>
> [...]
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index c192544e874b..743db1abec40 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -3777,7 +3777,7 @@ static int arm_smmu_device_hw_probe(struct
> > arm_smmu_device *smmu)
> > switch (FIELD_GET(IDR0_TTF, reg)) {
> > case IDR0_TTF_AARCH32_64:
> > smmu->ias = 40;
> > - fallthrough;
> > + break;
> > case IDR0_TTF_AARCH64:
> > break;
> > default:
>
> I have to say I don't really agree with the readability argument for
> this one - a fallthrough is semantically correct here, since the first
> case is a superset of the second. It just happens that anything we would
> do for the common subset is implicitly assumed (there are other
> potential cases we simply haven't added support for at the moment), thus
> the second case is currently empty.
> This change actively obfuscates that distinction.
Then perhaps comments should be added to usefully
describe the mechanisms.
case IDR0_TTF_AARCH32_64:
smmu->ias = 40;
fallthrough;/* and still do the 64 bit processing */
case IDR0_TTF_AARCH64:
/* Nothing specific yet */
break;
> Robin.
Re: [trivial PATCH] treewide: Convert switch/case fallthrough; to break;
On Wed, 2020-09-09 at 19:36 -0300, Jason Gunthorpe wrote:
> On Wed, Sep 09, 2020 at 01:06:39PM -0700, Joe Perches wrote:
> > fallthrough to a separate case/default label break; isn't very readable.
> >
> > Convert pseudo-keyword fallthrough; statements to a simple break; when
> > the next label is case or default and the only statement in the next
> > label block is break;
> >
> > Found using:
> >
> > $ grep-2.5.4 -rP --include=*.[ch] -n
> > "fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
> >
> > Miscellanea:
> >
> > o Move or coalesce a couple label blocks above a default: block.
> >
> > Signed-off-by: Joe Perches
> > ---
> >
> > Compiled allyesconfig x86-64 only.
> > A few files for other arches were not compiled.
>
> IB part looks OK, I prefer it like this
>
> You could do the same for continue as well, I saw a few of those..
I saw some continue uses as well but wasn't sure
and didn't look to see if the switch/case with
continue was in a for/while loop.
[trivial PATCH] treewide: Convert switch/case fallthrough; to break;
fallthrough to a separate case/default label break; isn't very readable.
Convert pseudo-keyword fallthrough; statements to a simple break; when
the next label is case or default and the only statement in the next
label block is break;
Found using:
$ grep-2.5.4 -rP --include=*.[ch] -n
"fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
Miscellanea:
o Move or coalesce a couple label blocks above a default: block.
Signed-off-by: Joe Perches
---
Compiled allyesconfig x86-64 only.
A few files for other arches were not compiled.
arch/arm/mach-mmp/pm-pxa910.c | 2 +-
arch/arm64/kvm/handle_exit.c | 2 +-
arch/mips/kernel/cpu-probe.c | 2 +-
arch/mips/math-emu/cp1emu.c | 2 +-
arch/s390/pci/pci.c | 2 +-
crypto/tcrypt.c | 4 ++--
drivers/ata/sata_mv.c | 2 +-
drivers/atm/lanai.c | 2 +-
drivers/gpu/drm/i915/display/intel_sprite.c | 2 +-
drivers/gpu/drm/nouveau/nvkm/engine/disp/hdmi.c | 2 +-
drivers/hid/wacom_wac.c | 2 +-
drivers/i2c/busses/i2c-i801.c | 2 +-
drivers/infiniband/ulp/rtrs/rtrs-clt.c| 14 +++---
drivers/infiniband/ulp/rtrs/rtrs-srv.c| 6 +++---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
drivers/irqchip/irq-vic.c | 4 ++--
drivers/md/dm.c | 2 +-
drivers/media/dvb-frontends/drxd_hard.c | 2 +-
drivers/media/i2c/ov5640.c| 2 +-
drivers/media/i2c/ov6650.c| 5 ++---
drivers/media/i2c/smiapp/smiapp-core.c| 2 +-
drivers/media/i2c/tvp5150.c | 2 +-
drivers/media/pci/ddbridge/ddbridge-core.c| 2 +-
drivers/media/usb/cpia2/cpia2_core.c | 2 +-
drivers/mfd/iqs62x.c | 3 +--
drivers/mmc/host/atmel-mci.c | 2 +-
drivers/mtd/nand/raw/nandsim.c| 2 +-
drivers/net/ethernet/intel/e1000e/phy.c | 2 +-
drivers/net/ethernet/intel/fm10k/fm10k_pf.c | 2 +-
drivers/net/ethernet/intel/i40e/i40e_adminq.c | 2 +-
drivers/net/ethernet/intel/i40e/i40e_txrx.c | 2 +-
drivers/net/ethernet/intel/iavf/iavf_txrx.c | 2 +-
drivers/net/ethernet/intel/igb/e1000_phy.c| 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c| 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c| 2 +-
drivers/net/ethernet/intel/ixgbevf/vf.c | 2 +-
drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c | 2 +-
drivers/net/ethernet/qlogic/qed/qed_mcp.c | 2 +-
drivers/net/ethernet/sfc/falcon/farch.c | 2 +-
drivers/net/ethernet/sfc/farch.c | 2 +-
drivers/net/phy/adin.c| 3 +--
drivers/net/usb/pegasus.c | 4 ++--
drivers/net/usb/usbnet.c | 2 +-
drivers/net/wireless/ath/ath5k/eeprom.c | 2 +-
drivers/net/wireless/mediatek/mt7601u/dma.c | 8
drivers/nvme/host/core.c | 12 ++--
drivers/pcmcia/db1xxx_ss.c| 4 ++--
drivers/power/supply/abx500_chargalg.c| 2 +-
drivers/power/supply/charger-manager.c| 2 +-
drivers/rtc/rtc-pcf85063.c| 2 +-
drivers/s390/scsi/zfcp_fsf.c | 2 +-
drivers/scsi/aic7xxx/aic79xx_core.c | 4 ++--
drivers/scsi/aic94xx/aic94xx_tmf.c| 2 +-
drivers/scsi/lpfc/lpfc_sli.c | 2 +-
drivers/scsi/smartpqi/smartpqi_init.c | 2 +-
drivers/scsi/sr.c | 2 +-
drivers/tty/serial/sunsu.c| 2 +-
drivers/tty/serial/sunzilog.c | 2 +-
drivers/tty/vt/vt_ioctl.c | 2 +-
drivers/usb/dwc3/core.c | 2 +-
drivers/usb/gadget/legacy/inode.c | 2 +-
drivers/usb/gadget/udc/pxa25x_udc.c | 4 ++--
drivers/usb/host/ohci-hcd.c | 2 +-
drivers/usb/isp1760/isp1760-hcd.c | 2 +-
drivers/usb/
Re: [PATCH][next] mt7601u: Use fallthrough pseudo-keyword
On Tue, 2020-09-01 at 12:36 -0500, Gustavo A. R. Silva wrote: > Replace the existing /* fall through */ comments and its variants with > the new pseudo-keyword macro fallthrough[1]. Also, remove unnecessary > fall-through markings when it is the case. [] > diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.c > b/drivers/net/wireless/mediatek/mt7601u/dma.c [] > @@ -196,7 +196,7 @@ static void mt7601u_complete_rx(struct urb *urb) > default: > dev_err_ratelimited(dev->dev, "rx urb failed: %d\n", > urb->status); > - /* fall through */ > + fallthrough; fallthrough to break is odd. break would probably be better. > case 0: > break; > } > @@ -241,7 +241,7 @@ static void mt7601u_complete_tx(struct urb *urb) > default: > dev_err_ratelimited(dev->dev, "tx urb failed: %d\n", > urb->status); > - /* fall through */ > + fallthrough; here too... > case 0: > break; > } >
Re: [net-next] net: iavf: Use the ARRAY_SIZE macro for aq_to_posix
On Wed, 2020-09-09 at 02:33 -0700, Joe Perches wrote: > On Wed, 2020-09-09 at 16:51 +0800, Wei Xu wrote: > > Use the ARRAY_SIZE macro to calculate the size of an array. > > This code was detected with the help of Coccinelle. > [] > > diff --git a/drivers/net/ethernet/intel/iavf/iavf_adminq.h > > b/drivers/net/ethernet/intel/iavf/iavf_adminq.h > [] > > @@ -120,7 +120,7 @@ static inline int iavf_aq_rc_to_posix(int aq_ret, int > > aq_rc) > > if (aq_ret == IAVF_ERR_ADMIN_QUEUE_TIMEOUT) > > return -EAGAIN; > > > > - if (!((u32)aq_rc < (sizeof(aq_to_posix) / sizeof((aq_to_posix)[0] > > + if (!((u32)aq_rc < ARRAY_SIZE(aq_to_posix))) > > return -ERANGE; > > If you want to use a cast, > > if ((u32)aq_rc >= ARRAY_SIZE(aq_to_posix)) > return -ERANGE; > > would be a more common and simpler style, though > perhaps testing ac_rc < 0 would be more intelligible. > > if (ac_rc < 0 || ac_rq >= ARRAY_SIZE(aq_to_posix)) (hah, I typed aq_rc wrong both times, so maybe it's not _that_ much better...) if (aq_rc < 0 || aq_rc >= ARRAY_SIZE(aq_to_posix))
Re: [net-next] net: iavf: Use the ARRAY_SIZE macro for aq_to_posix
On Wed, 2020-09-09 at 16:51 +0800, Wei Xu wrote: > Use the ARRAY_SIZE macro to calculate the size of an array. > This code was detected with the help of Coccinelle. [] > diff --git a/drivers/net/ethernet/intel/iavf/iavf_adminq.h > b/drivers/net/ethernet/intel/iavf/iavf_adminq.h [] > @@ -120,7 +120,7 @@ static inline int iavf_aq_rc_to_posix(int aq_ret, int > aq_rc) > if (aq_ret == IAVF_ERR_ADMIN_QUEUE_TIMEOUT) > return -EAGAIN; > > - if (!((u32)aq_rc < (sizeof(aq_to_posix) / sizeof((aq_to_posix)[0] > + if (!((u32)aq_rc < ARRAY_SIZE(aq_to_posix))) > return -ERANGE; If you want to use a cast, if ((u32)aq_rc >= ARRAY_SIZE(aq_to_posix)) return -ERANGE; would be a more common and simpler style, though perhaps testing ac_rc < 0 would be more intelligible. if (ac_rc < 0 || ac_rq >= ARRAY_SIZE(aq_to_posix)) return -ERANGE;
Re: [PATCH net] net: phy: dp83867: Fix various styling and space issues
On Thu, 2020-09-03 at 11:41 -0500, Dan Murphy wrote: > On 9/3/20 11:34 AM, Florian Fainelli wrote: > > On 9/3/2020 7:15 AM, Dan Murphy wrote: > > > Fix spacing issues reported for misaligned switch..case and extra new > > > lines. [] > > > diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c [] > > > @@ -35,7 +34,7 @@ > > > #define DP83867_CFG4_SGMII_ANEG_MASK (BIT(5) | BIT(6)) > > > #define DP83867_CFG4_SGMII_ANEG_TIMER_11MS (3 << 5) > > > #define DP83867_CFG4_SGMII_ANEG_TIMER_800US (2 << 5) > > > -#define DP83867_CFG4_SGMII_ANEG_TIMER_2US(1 << 5) > > > +#define DP83867_CFG4_SGMII_ANEG_TIMER_2USBIT(5) > > Now the definitions are inconsistent, you would want to drop this one > > and stick to the existing style. > > OK I was a little conflicted making that change due to the reasons you > mentioned. But if that is an acceptable warning I am ok with it. Maybe use GENMASK for DP83867_CFG4_SGMII_ANEG_MASK instead.
