Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
On Tue, Apr 20, 2021 at 11:14 PM Vineet Gupta
wrote:
> On 4/20/21 12:07 AM, Arnd Bergmann wrote:
> >
> > which means that half the 32-bit architectures do this. This may
> > cause more problems when arc and/or microblaze want to support
> > 64-bit kernels and compat mode in the future on their latest hardware,
> > as that means duplicating the x86 specific hacks we have for compat.
> >
> > What is alignof(u64) on 64-bit arc?
>
> $ echo 'int a = __alignof__(long long);' | arc64-linux-gnu-gcc -xc -
> -Wall -S -o - | grep -A1 a: | tail -n 1 | cut -f 3
> 8
Ok, good.
> Yeah ARCv2 alignment of 4 for 64-bit data was a bit of surprise finding
> for me as well. When 64-bit load/stores were initially targeted by the
> internal Metaware compiler (llvm based) they decided to keep alignment
> to 4 still (granted hardware allowed this) and then gcc guys decided to
> follow the same ABI. I only found this by accident :-)
>
> Can you point me to some specifics on the compat issue. For better of
> worse, arc64 does''t have a compat 32-bit mode, so everything is
> 64-on-64 or 32-on-32 (ARC32 flavor of ARCv3)
In that case, there should be no problem for you.
The main issue is with system calls and ioctls that contain a misaligned
struct member like
struct s {
u32 a;
u64 b;
};
Passing this structure by reference from a 32-bit user space application
to a 64-bit kernel with different alignment constraints means that the
kernel has to convert the structure layout. See
compat_ioctl_preallocate() in fs/ioctl.c for one such example.
Arnd
Re: [RFC v1 PATCH 3/3] driver: update all the code that use soc_device_match
On Tue, Apr 20, 2021 at 1:44 AM Dominique MARTINET wrote: > Arnd Bergmann wrote on Mon, Apr 19, 2021 at 02:16:36PM +0200: > > For built-in drivers, load order depends on the initcall level and > > link order (how things are lined listed in the Makefile hierarchy). > > > > For loadable modules, this is up to user space in the end. > > > > Which of the drivers in this scenario are loadable modules? > > All the drivers involved in my case are built-in (nvmem, soc and final > soc_device_match consumer e.g. caam_jr that crashes the kernel if soc is > not identified properly). Ok, in that case you may have a chance to just adapt the initcall levels. This is somewhat fragile if someone else already relies on a particular order, but it's an easy one-line change to change a driver e.g. from module_init() or device_initcall() to arch_initcall(). > I frankly don't like the idea of moving nvmem/ above soc/ in > drivers/Makefile as a "solution" to this (especially as there is one > that seems to care about what soc they run on...), so I'll have a look > at links first, hopefully that will work out. Right, that would be way more fragile. I think the main problem in this case is the caam driver that really should not look into the particular SoC type or even machine compatible string. This is something we can do as a last resort for compatibility with busted devicetree files, but it appears that this driver does it as the primary method for identifying different hardware revisions. I would suggest fixing the binding so that each SoC that includes one of these devices has a soc specific compatible string associated with the device that the driver can use as the primary way of identifying the device. We probably need to keep the old logic around for old dtb files, but there can at least be a comment next to that table that discourages people from adding more entries there. Arnd
Re: [RFC v1 PATCH 3/3] driver: update all the code that use soc_device_match
On Tue, Apr 20, 2021 at 1:44 AM Dominique MARTINET wrote: > Arnd Bergmann wrote on Mon, Apr 19, 2021 at 02:16:36PM +0200: > > For built-in drivers, load order depends on the initcall level and > > link order (how things are lined listed in the Makefile hierarchy). > > > > For loadable modules, this is up to user space in the end. > > > > Which of the drivers in this scenario are loadable modules? > > All the drivers involved in my case are built-in (nvmem, soc and final > soc_device_match consumer e.g. caam_jr that crashes the kernel if soc is > not identified properly). Ok, in that case you may have a chance to just adapt the initcall levels. This is somewhat fragile if someone else already relies on a particular order, but it's an easy one-line change to change a driver e.g. from module_init() or device_initcall() to arch_initcall(). > I frankly don't like the idea of moving nvmem/ above soc/ in > drivers/Makefile as a "solution" to this (especially as there is one > that seems to care about what soc they run on...), so I'll have a look > at links first, hopefully that will work out. Right, that would be way more fragile. I think the main problem in this case is the caam driver that really should not look into the particular SoC type or even machine compatible string. This is something we can do as a last resort for compatibility with busted devicetree files, but it appears that this driver does it as the primary method for identifying different hardware revisions. I would suggest fixing the binding so that each SoC that includes one of these devices has a soc specific compatible string associated with the device that the driver can use as the primary way of identifying the device. We probably need to keep the old logic around for old dtb files, but there can at least be a comment next to that table that discourages people from adding more entries there. Arnd
Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
On Tue, Apr 20, 2021 at 5:10 AM Matthew Wilcox wrote:
>
> On Tue, Apr 20, 2021 at 02:48:17AM +, Vineet Gupta wrote:
> > > 32-bit architectures which expect 8-byte alignment for 8-byte integers
> > > and need 64-bit DMA addresses (arc, arm, mips, ppc) had their struct
> > > page inadvertently expanded in 2019.
> >
> > FWIW, ARC doesn't require 8 byte alignment for 8 byte integers. This is
> > only needed for 8-byte atomics due to the requirements of LLOCKD/SCOND
> > instructions.
>
> Ah, like x86? OK, great, I'll drop your arch from the list of
> affected. Thanks!
I mistakenly assumed that i386 and m68k were the only supported
architectures with 32-bit alignment on u64. I checked it now and found
$ for i in /home/arnd/cross/x86_64/gcc-10.1.0-nolibc/*/bin/*-gcc ; do
echo `echo 'int a = __alignof__(long long);' | $i -xc - -Wall -S -o- |
grep -A1 a: | tail -n 1 | cut -f 3 -d\ `
${i#/home/arnd/cross/x86_64/gcc-10.1.0-nolibc/*/bin/} ; done
8 aarch64-linux-gcc
8 alpha-linux-gcc
4 arc-linux-gcc
8 arm-linux-gnueabi-gcc
8 c6x-elf-gcc
4 csky-linux-gcc
4 h8300-linux-gcc
8 hppa-linux-gcc
8 hppa64-linux-gcc
8 i386-linux-gcc
8 ia64-linux-gcc
2 m68k-linux-gcc
4 microblaze-linux-gcc
8 mips-linux-gcc
8 mips64-linux-gcc
8 nds32le-linux-gcc
4 nios2-linux-gcc
4 or1k-linux-gcc
8 powerpc-linux-gcc
8 powerpc64-linux-gcc
8 riscv32-linux-gcc
8 riscv64-linux-gcc
8 s390-linux-gcc
4 sh2-linux-gcc
4 sh4-linux-gcc
8 sparc-linux-gcc
8 sparc64-linux-gcc
8 x86_64-linux-gcc
8 xtensa-linux-gcc
which means that half the 32-bit architectures do this. This may
cause more problems when arc and/or microblaze want to support
64-bit kernels and compat mode in the future on their latest hardware,
as that means duplicating the x86 specific hacks we have for compat.
What is alignof(u64) on 64-bit arc?
Arnd
Re: [PATCH][next] wlcore: Fix buffer overrun by snprintf due to incorrect buffer size Content-Type: text/plain; charset="utf-8"
On Mon, Apr 19, 2021 at 4:01 PM Colin King wrote:
>
> From: Colin Ian King
>
> The size of the buffer than can be written to is currently incorrect, it is
> always the size of the entire buffer even though the snprintf is writing
> as position pos into the buffer. Fix this by setting the buffer size to be
> the number of bytes left in the buffer, namely sizeof(buf) - pos.
>
> Addresses-Coverity: ("Out-of-bounds access")
> Fixes: 7b0e2c4f6be3 ("wlcore: fix overlapping snprintf arguments in debugfs")
> Signed-off-by: Colin Ian King
Acked-by: Arnd Bergmann
Re: [RFC v1 PATCH 3/3] driver: update all the code that use soc_device_match
On Mon, Apr 19, 2021 at 11:33 AM Dominique MARTINET
wrote:
> Geert Uytterhoeven wrote on Mon, Apr 19, 2021 at 11:03:24AM +0200:
>
> > soc_device_match() should only be used as a last resort, to identify
> > systems that cannot be identified otherwise. Typically this is used for
> > quirks, which should only be enabled on a very specific subset of
> > systems. IMHO such systems should make sure soc_device_match()
> > is available early, by registering their SoC device early.
>
> I definitely agree there, my suggestion to defer was only because I know
> of no other way to influence the ordering of drivers loading reliably
> and gave up on soc being init'd early.
In some cases, you can use the device_link infrastructure to deal
with dependencies between devices. Not sure if this would help
in your case, but have a look at device_link_add() etc in drivers/base/core.c
> In this particular case the problem is that since 7d981405d0fd ("soc:
> imx8m: change to use platform driver") the soc probe tries to use the
> nvmem driver for ocotp fuses for imx8m devices, which isn't ready yet.
> So soc loading gets pushed back to the end of the list because it gets
> defered and other drivers relying on soc_device_match get confused
> because they wrongly think a device doesn't match a quirk when it
> actually does.
>
> If there is a way to ensure the nvmem driver gets loaded before the soc,
> that would also solve the problem nicely, and avoid the need to mess
> with all the ~50 drivers which use it.
>
> Is there a way to control in what order drivers get loaded? Something in
> the dtb perhaps?
For built-in drivers, load order depends on the initcall level and
link order (how things are lined listed in the Makefile hierarchy).
For loadable modules, this is up to user space in the end.
Which of the drivers in this scenario are loadable modules?
Arnd
Re: [PATCH 1/1] mm: Fix struct page layout on 32-bit systems
On Sat, Apr 17, 2021 at 3:58 PM Matthew Wilcox wrote: > I wouldn't like to make that assumption. I've come across IOMMUs (maybe > on parisc? powerpc?) that like to encode fun information in the top > few bits. So we could get it down to 52 bits, but I don't think we can > get all the way down to 32 bits. Also, we need to keep the bottom bit > clear for PageTail, so that further constrains us. I'd be surprised to find such an IOMMU on a 32-bit machine, given that the main reason for using an IOMMU on these is to avoid the 32-bit address limit in DMA masters. I see that parisc32 does not enable 64-bit dma_addr_t, while powerpc32 does not support any IOMMU, so it wouldn't be either of those two. I do remember some powerpc systems that encode additional flags (transaction ordering, caching, ...) into the high bits of the physical address in the IOTLB, but not the virtual address used for looking them up. > Anyway, I like the "two unsigned longs" approach I posted yesterday, > but thanks for the suggestion. Ok, fair enough. As long as there are enough bits in this branch of 'struct page', I suppose it is the safe choice. Arnd
Re: [PATCH 1/1] mm: Fix struct page layout on 32-bit systems
On Fri, Apr 16, 2021 at 5:27 PM Matthew Wilcox wrote:
> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> index b5b195305346..db7c7020746a 100644
> --- a/include/net/page_pool.h
> +++ b/include/net/page_pool.h
> @@ -198,7 +198,17 @@ static inline void page_pool_recycle_direct(struct
> page_pool *pool,
>
> static inline dma_addr_t page_pool_get_dma_addr(struct page *page)
> {
> - return page->dma_addr;
> + dma_addr_t ret = page->dma_addr[0];
> + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> + ret |= (dma_addr_t)page->dma_addr[1] << 32;
> + return ret;
> +}
Have you considered using a PFN type address here? I suspect you
can prove that shifting the DMA address by PAGE_BITS would
make it fit into an 'unsigned long' on all 32-bit architectures with
64-bit dma_addr_t. This requires that page->dma_addr to be
page aligned, as well as fit into 44 bits. I recently went through the
maximum address space per architecture to define a
MAX_POSSIBLE_PHYSMEM_BITS, and none of them have more than
40 here, presumably the same is true for dma address space.
Arnd
Re: Bogus struct page layout on 32-bit
On Fri, Apr 16, 2021 at 11:27 AM 'Grygorii Strashko' via Clang Built
Linux wrote:
> On 10/04/2021 11:52, Ilias Apalodimas wrote:
> > +CC Grygorii for the cpsw part as Ivan's email is not valid anymore
> The TI platforms am3/4/5 (cpsw) and Keystone 2 (netcp) can do only 32bit DMA
> even in case of LPAE (dma-ranges are used).
> Originally, as I remember, CONFIG_ARCH_DMA_ADDR_T_64BIT has not been selected
> for the LPAE case
> on TI platforms and the fact that it became set is the result of
> multi-paltform/allXXXconfig/DMA
> optimizations and unification.
> (just checked - not set in 4.14)
>
> Probable commit 4965a68780c5 ("arch: define the ARCH_DMA_ADDR_T_64BIT config
> symbol in lib/Kconfig").
I completely missed this change in the past, and I don't really agree
with it either.
Most 32-bit Arm platforms are in fact limited to 32-bit DMA, even when they have
MMIO or RAM areas above the 4GB boundary that require LPAE.
> The TI drivers have been updated, finally to accept ARCH_DMA_ADDR_T_64BIT=y
> by using
> things like (__force u32) for example.
>
> Honestly, I've done sanity check of CPSW with LPAE=y
> (ARCH_DMA_ADDR_T_64BIT=y) very long time ago.
This is of course a good idea, drivers should work with any
combination of 32-bit
or 64-bit phys_addr_t and dma_addr_t.
Arnd
Re: [PATCH net-next] net: Space: remove hp100 probe
On Wed, Apr 14, 2021, 00:42 Stephen Hemminger
wrote:
>
> On Tue, 13 Apr 2021 16:16:17 +0200 Arnd Bergmann wrote:
>
> > */
> > static struct devprobe2 isa_probes[] __initdata = {
> > -#if defined(CONFIG_HP100) && defined(CONFIG_ISA) /* ISA, EISA */
> > - {hp100_probe, 0},
> > -#endif
> > #ifdef CONFIG_3C515
> > {tc515_probe, 0},
> > #endif
>
> Thanks, do we even need to have the static initialization anymore?
I actually did some more cleanups after I sent the above patch when
I found out that this code still exists. It turned out that above half of
the static initializations are completely pointless because the
drivers never rely on the netdev= command line arguments and
can simply be changed to always using module_init() instead of
relying on net_olddevs_init() for the built-in case.
The remaining ones are all ISA drivers: 3c515, Ultra, WD80x3,
NE2000, Lance, SMC9194, CS89x0, NI65 and COPS.
With my cleanups, I move the netdev_boot_setup infrastructure
into drivers/net/Space.c and only compile it when at least one of
these eight drivers is enabled.
All these drivers also support being built as loadable modules, but
in that configuration they only support a single device (back in the
day you could copy the module and just load it twice to support
more than one instance, not sure we still want to support that).
None of these drivers have a maintainer listed, but I suppose
there are still some PC/104 machines with NE2000 network
cards that could theoretically run a modern kernel.
Arnd
[PATCH net-next] net: Space: remove hp100 probe
From: Arnd Bergmann
The driver was removed last year, but the static initialization got left
behind by accident.
Fixes: a10079c66290 ("staging: remove hp100 driver")
Signed-off-by: Arnd Bergmann
---
drivers/net/Space.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/Space.c b/drivers/net/Space.c
index 7bb699d7c422..a61cc7b26a87 100644
--- a/drivers/net/Space.c
+++ b/drivers/net/Space.c
@@ -59,9 +59,6 @@ static int __init probe_list2(int unit, struct devprobe2 *p,
int autoprobe)
* look for EISA/PCI cards in addition to ISA cards).
*/
static struct devprobe2 isa_probes[] __initdata = {
-#if defined(CONFIG_HP100) && defined(CONFIG_ISA) /* ISA, EISA */
- {hp100_probe, 0},
-#endif
#ifdef CONFIG_3C515
{tc515_probe, 0},
#endif
--
2.29.2
Re: [PATCH kbuild] Makefile.extrawarn: disable -Woverride-init in W=1
On Wed, Apr 7, 2021 at 2:24 AM Marek Behún wrote:
>
> The -Wextra flag enables -Woverride-init in newer versions of GCC.
>
> This causes the compiler to warn when a value is written twice in a
> designated initializer, for example:
> int x[1] = {
> [0] = 3,
> [0] = 3,
> };
>
> Note that for clang, this was disabled from the beginning with
> -Wno-initializer-overrides in commit a1494304346a3 ("kbuild: add all
> Clang-specific flags unconditionally").
>
> This prevents us from implementing complex macros for compile-time
> initializers.
I think this is generally a useful warning, and it has found a number
of real bugs. I would want this to be enabled in both gcc and clang
by default, and I have previously sent both bugfixes and patches to
disable it locally.
> For example a macro of the form INITIALIZE_BITMAP(bits...) that can be
> used as
> static DECLARE_BITMAP(bm, 64) = INITIALIZE_BITMAP(0, 1, 32, 33);
> can only be implemented by allowing a designated initializer to
> initialize the same members multiple times (because the compiler
> complains even if the multiple initializations initialize to the same
> value).
We don't have this kind of macro at the moment, and this may just mean
you need to try harder to come up with a definition that only initializes
each member once if you want to add this.
How do you currently define it?
Arnd
Re: [PATCH 06/11] cgroup: fix -Wzero-length-bounds warnings
On Tue, Mar 30, 2021 at 10:41 AM Michal Koutný wrote: > > On Mon, Mar 22, 2021 at 05:02:44PM +0100, Arnd Bergmann > wrote: > > I'm not sure what is expected to happen for such a configuration, > > presumably these functions are never calls in that case. > Yes, the functions you patched would only be called from subsystems or > there should be no way to obtain a struct cgroup_subsys reference > anyway (hence it's ok to always branch as if ss==NULL). > > I'd prefer a variant that wouldn't compile the affected codepaths when > there are no subsystems registered, however, I couldn't come up with a > way how to do it without some preprocessor ugliness. Would it be possible to enclose most or all of kernel/cgroup/cgroup.c in an #ifdef CGROUP_SUBSYS_COUNT block? I didn't try that myself, but this might be a way to guarantee that there cannot be any callers (it would cause a link error). > Reviewed-by: Michal Koutný Thanks Arnd
Re: [net-next V2 01/13] net/mlx5e: alloc the correct size for indirection_rqt
On Fri, Mar 26, 2021 at 3:53 AM Saeed Mahameed wrote:
>
> From: Saeed Mahameed
>
> The cited patch allocated the wrong size for the indirection_rqt table,
> fix that.
>
> Fixes: 2119bda642c4 ("net/mlx5e: allocate 'indirection_rqt' buffer
> dynamically")
> CC: Arnd Bergmann
> Signed-off-by: Saeed Mahameed
Thanks for fixing my mistake
Acked-by: Arnd Bergmann
Re: [PATCH 11/11] [RFC] drm/i915/dp: fix array overflow warning
On Thu, Mar 25, 2021 at 9:05 AM Jani Nikula wrote:
> > Clearly something is wrong here, but I can't quite figure out what.
> > Changing the array size to 16 bytes avoids the warning, but is
> > probably the wrong solution here.
>
> Ugh. drm_dp_channel_eq_ok() does not actually require more than
> DP_LINK_STATUS_SIZE - 2 elements in the link_status. It's some other
> related functions that do, and in most cases it's convenient to read all
> those DP_LINK_STATUS_SIZE bytes.
>
> However, here the case is slightly different for DP MST, and the change
> causes reserved DPCD addresses to be read. Not sure it matters, but
> really I think the problem is what drm_dp_channel_eq_ok() advertizes.
>
> I also don't like the array notation with sizes in function parameters
> in general, because I think it's misleading. Would gcc-11 warn if a
> function actually accesses the memory out of bounds of the size?
Yes, that is the point of the warning. Using an explicit length in an
array argument type tells gcc that the function will never access
beyond the end of that bound, and that passing a short array
is a bug.
I don't know if this /only/ means triggering a warning, or if gcc
is also able to make optimizations after classifying this as undefined
behavior that it would not make for an unspecified length.
> Anyway. I don't think we're going to get rid of the array notation
> anytime soon, if ever, no matter how much I dislike it, so I think the
> right fix would be to at least state the correct required size in
> drm_dp_channel_eq_ok().
Ok. Just to confirm: Changing the declaration to an unspecified length
avoids the warnings, as does the patch below:
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index eedbb48815b7..6ebeec3d88a7 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -46,12 +46,12 @@
*/
/* Helpers for DP link training */
-static u8 dp_link_status(const u8 link_status[DP_LINK_STATUS_SIZE], int r)
+static u8 dp_link_status(const u8 link_status[DP_LINK_STATUS_SIZE - 2], int r)
{
return link_status[r - DP_LANE0_1_STATUS];
}
-static u8 dp_get_lane_status(const u8 link_status[DP_LINK_STATUS_SIZE],
+static u8 dp_get_lane_status(const u8 link_status[DP_LINK_STATUS_SIZE - 2],
int lane)
{
int i = DP_LANE0_1_STATUS + (lane >> 1);
@@ -61,7 +61,7 @@ static u8 dp_get_lane_status(const u8
link_status[DP_LINK_STATUS_SIZE],
return (l >> s) & 0xf;
}
-bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
+bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE - 2],
int lane_count)
{
u8 lane_align;
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index edffd1dcca3e..160f7fd127b1 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1456,7 +1456,7 @@ enum drm_dp_phy {
#define DP_LINK_CONSTANT_N_VALUE 0x8000
#define DP_LINK_STATUS_SIZE 6
-bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
+bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE - 2],
int lane_count);
bool drm_dp_clock_recovery_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
int lane_count);
This obviously needs a good explanation in the code and the changelog text,
which I don't have, but if the above is what you had in mind, please take that
and add Reported-by/Tested-by: Arnd Bergmann .
Arnd
Re: [PATCH net-next 5/5] vxge: avoid -Wemtpy-body warnings
On Mon, Mar 22, 2021 at 11:43 AM Arnd Bergmann wrote:
>
> From: Arnd Bergmann
>
> There are a few warnings about empty debug macros in this driver:
>
> drivers/net/ethernet/neterion/vxge/vxge-main.c: In function 'vxge_probe':
> drivers/net/ethernet/neterion/vxge/vxge-main.c:4480:76: error: suggest braces
> around empty body in an 'if' statement [-Werror=empty-body]
> 4480 | "Failed in enabling SRIOV mode:
> %d\n", ret);
>
> Change them to proper 'do { } while (0)' expressions to make the
> code a little more robust and avoid the warnings.
>
> Signed-off-by: Arnd Bergmann
Please disregard this patch, I was accidentally building without -Wformat and
failed to notice that this introduces a regression. I'll send a new
version after
more testing.
Arnd
Re: [PATCH] [v2] hinic: avoid gcc -Wrestrict warning
On Wed, Mar 24, 2021 at 2:29 PM Rasmus Villemoes
wrote:
> On 24/03/2021 14.07, Arnd Bergmann wrote:
>
> > if (set_settings & HILINK_LINK_SET_SPEED) {
> > speed_level = hinic_ethtool_to_hw_speed_level(speed);
> > err = snprintf(set_link_str, SET_LINK_STR_MAX_LEN,
> > -"%sspeed %d ", set_link_str, speed);
> > - if (err <= 0 || err >= SET_LINK_STR_MAX_LEN) {
> > +"speed %d ", speed);
> > + if (err >= SET_LINK_STR_MAX_LEN) {
> > netif_err(nic_dev, drv, netdev, "Failed to snprintf
> > link speed, function return(%d) and dest_len(%d)\n",
> > err, SET_LINK_STR_MAX_LEN);
> > return -EFAULT;
>
> It's not your invention of course, but this both seems needlessly harsh
> and EFAULT is a weird error to return. It's just a printk() message that
> might be truncated, and now that the format string only has a %d
> specifier, it can actually be verified statically that overflow will
> never happen (though I don't know or think gcc can do that, perhaps
> there's some locale nonsense in the standard that allows using
> utf16-encoded sanskrit runes). So probably that test should just be
> dropped, but that's a separate thing.
I thought about fixing it, but this seemed to be a rabbit hole I didn't
want to get into, as there are other harmless issues in the driver
that could be improved.
I'm fairly sure gcc can indeed warn about the overflow with
-Wformat-truncation, but the warning is disabled at the moment
because it has a ton of false positives:
kernel/workqueue.c: In function 'create_worker':
kernel/workqueue.c:1933:55: error: '%d' directive output may be
truncated writing between 1 and 10 bytes into a region of size between
3 and 13 [-Werror=format-truncation=]
1933 | snprintf(id_buf, sizeof(id_buf), "u%d:%d",
pool->id, id);
Arnd
[PATCH] [v2] hinic: avoid gcc -Wrestrict warning
From: Arnd Bergmann
With extra warnings enabled, gcc complains that snprintf should not
take the same buffer as source and destination:
drivers/net/ethernet/huawei/hinic/hinic_ethtool.c: In function
'hinic_set_settings_to_hw':
drivers/net/ethernet/huawei/hinic/hinic_ethtool.c:480:9: error: 'snprintf'
argument 4 overlaps destination object 'set_link_str' [-Werror=restrict]
480 | err = snprintf(set_link_str, SET_LINK_STR_MAX_LEN,
| ^~~~
481 | "%sspeed %d ", set_link_str, speed);
| ~~~
drivers/net/ethernet/huawei/hinic/hinic_ethtool.c:464:7: note: destination
object referenced by 'restrict'-qualified argument 1 was declared here
464 | char set_link_str[SET_LINK_STR_MAX_LEN] = {0};
Rewrite this to avoid the nested sprintf and instead use separate
buffers, which is simpler.
Cc: Rasmus Villemoes
Signed-off-by: Arnd Bergmann
---
v2: rework according to feedback from Rasmus. This one could
easily avoid most of the pitfalls
---
.../net/ethernet/huawei/hinic/hinic_ethtool.c | 25 ---
1 file changed, 10 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/huawei/hinic/hinic_ethtool.c
b/drivers/net/ethernet/huawei/hinic/hinic_ethtool.c
index c340d9acba80..d7e20dab6e48 100644
--- a/drivers/net/ethernet/huawei/hinic/hinic_ethtool.c
+++ b/drivers/net/ethernet/huawei/hinic/hinic_ethtool.c
@@ -34,7 +34,7 @@
#include "hinic_rx.h"
#include "hinic_dev.h"
-#define SET_LINK_STR_MAX_LEN 128
+#define SET_LINK_STR_MAX_LEN 16
#define GET_SUPPORTED_MODE 0
#define GET_ADVERTISED_MODE1
@@ -462,24 +462,19 @@ static int hinic_set_settings_to_hw(struct hinic_dev
*nic_dev,
{
struct hinic_link_ksettings_info settings = {0};
char set_link_str[SET_LINK_STR_MAX_LEN] = {0};
+ const char *autoneg_str;
struct net_device *netdev = nic_dev->netdev;
enum nic_speed_level speed_level = 0;
int err;
- err = snprintf(set_link_str, SET_LINK_STR_MAX_LEN, "%s",
- (set_settings & HILINK_LINK_SET_AUTONEG) ?
- (autoneg ? "autong enable " : "autong disable ") : "");
- if (err < 0 || err >= SET_LINK_STR_MAX_LEN) {
- netif_err(nic_dev, drv, netdev, "Failed to snprintf link state,
function return(%d) and dest_len(%d)\n",
- err, SET_LINK_STR_MAX_LEN);
- return -EFAULT;
- }
+ autoneg_str = (set_settings & HILINK_LINK_SET_AUTONEG) ?
+ (autoneg ? "autong enable " : "autong disable ") : "";
if (set_settings & HILINK_LINK_SET_SPEED) {
speed_level = hinic_ethtool_to_hw_speed_level(speed);
err = snprintf(set_link_str, SET_LINK_STR_MAX_LEN,
- "%sspeed %d ", set_link_str, speed);
- if (err <= 0 || err >= SET_LINK_STR_MAX_LEN) {
+ "speed %d ", speed);
+ if (err >= SET_LINK_STR_MAX_LEN) {
netif_err(nic_dev, drv, netdev, "Failed to snprintf
link speed, function return(%d) and dest_len(%d)\n",
err, SET_LINK_STR_MAX_LEN);
return -EFAULT;
@@ -494,11 +489,11 @@ static int hinic_set_settings_to_hw(struct hinic_dev
*nic_dev,
err = hinic_set_link_settings(nic_dev->hwdev, &settings);
if (err != HINIC_MGMT_CMD_UNSUPPORTED) {
if (err)
- netif_err(nic_dev, drv, netdev, "Set %s failed\n",
- set_link_str);
+ netif_err(nic_dev, drv, netdev, "Set %s%sfailed\n",
+ autoneg_str, set_link_str);
else
- netif_info(nic_dev, drv, netdev, "Set %s
successfully\n",
- set_link_str);
+ netif_info(nic_dev, drv, netdev, "Set
%s%ssuccessfully\n",
+ autoneg_str, set_link_str);
return err;
}
--
2.29.2
[PATCH net-next] [RESEND] ch_ktls: fix enum-conversion warning
From: Arnd Bergmann
gcc points out an incorrect enum assignment:
drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c: In function
'chcr_ktls_cpl_set_tcb_rpl':
drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c:684:22: warning:
implicit conversion from 'enum ' to 'enum ch_ktls_open_state'
[-Wenum-conversion]
This appears harmless, and should apparently use 'CH_KTLS_OPEN_SUCCESS'
instead of 'false', with the same value '0'.
Fixes: efca3878a5fb ("ch_ktls: Issue if connection offload fails")
Reviewed-by: Andrew Lunn
Signed-off-by: Arnd Bergmann
---
I sent this last October, but it never made it in, resending
---
drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
index 46a809f2aeca..7c599195e256 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
@@ -722,7 +722,7 @@ static int chcr_ktls_cpl_set_tcb_rpl(struct adapter *adap,
unsigned char *input)
kvfree(tx_info);
return 0;
}
- tx_info->open_state = false;
+ tx_info->open_state = CH_KTLS_OPEN_SUCCESS;
spin_unlock(&tx_info->lock);
complete(&tx_info->completion);
--
2.29.2
Re: [RFC net] net: skbuff: fix stack variable out of bounds access
On Tue, Mar 23, 2021 at 3:42 PM Willem de Bruijn
wrote:
>
> On Tue, Mar 23, 2021 at 8:52 AM Arnd Bergmann wrote:
> >>
> A similar fix already landed in 5.12-rc3: commit b228c9b05876 ("net:
> expand textsearch ts_state to fit skb_seq_state"). That fix landed in
> 5.12-rc3.
Ah nice, even the same BUILD_BUG_ON() ;-)
Too bad it had to be found through runtime testing when it could have been
found by the compiler warning.
Arnd
[PATCH net-next] airo: work around stack usage warning
From: Arnd Bergmann
gcc-11 with KASAN on 32-bit arm produces a warning about a function
that needs a lot of stack space:
drivers/net/wireless/cisco/airo.c: In function 'setup_card.constprop':
drivers/net/wireless/cisco/airo.c:3960:1: error: the frame size of 1512 bytes
is larger than 1400 bytes [-Werror=frame-larger-than=]
Most of this is from a single large structure that could be dynamically
allocated or moved into the per-device structure. However, as the callers
all seem to have a fairly well bounded call chain, the easiest change
is to pull out the part of the function that needs the large variables
into a separate function and mark that as noinline_for_stack. This does
not reduce the total stack usage, but it gets rid of the warning and
requires minimal changes otherwise.
Signed-off-by: Arnd Bergmann
---
drivers/net/wireless/cisco/airo.c | 117 +-
1 file changed, 65 insertions(+), 52 deletions(-)
diff --git a/drivers/net/wireless/cisco/airo.c
b/drivers/net/wireless/cisco/airo.c
index e35e1380ae43..540ba694899c 100644
--- a/drivers/net/wireless/cisco/airo.c
+++ b/drivers/net/wireless/cisco/airo.c
@@ -3818,6 +3818,68 @@ static inline void set_auth_type(struct airo_info
*local, int auth_type)
local->last_auth = auth_type;
}
+static int noinline_for_stack airo_readconfig(struct airo_info *ai, u8 *mac,
int lock)
+{
+ int i, status;
+ /* large variables, so don't inline this function,
+* maybe change to kmalloc
+*/
+ tdsRssiRid rssi_rid;
+ CapabilityRid cap_rid;
+
+ kfree(ai->SSID);
+ ai->SSID = NULL;
+ // general configuration (read/modify/write)
+ status = readConfigRid(ai, lock);
+ if (status != SUCCESS) return ERROR;
+
+ status = readCapabilityRid(ai, &cap_rid, lock);
+ if (status != SUCCESS) return ERROR;
+
+ status = PC4500_readrid(ai, RID_RSSI, &rssi_rid, sizeof(rssi_rid),
lock);
+ if (status == SUCCESS) {
+ if (ai->rssi || (ai->rssi = kmalloc(512, GFP_KERNEL)) != NULL)
+ memcpy(ai->rssi, (u8*)&rssi_rid + 2, 512); /* Skip RID
length member */
+ }
+ else {
+ kfree(ai->rssi);
+ ai->rssi = NULL;
+ if (cap_rid.softCap & cpu_to_le16(8))
+ ai->config.rmode |= RXMODE_NORMALIZED_RSSI;
+ else
+ airo_print_warn(ai->dev->name, "unknown received signal
"
+ "level scale");
+ }
+ ai->config.opmode = adhoc ? MODE_STA_IBSS : MODE_STA_ESS;
+ set_auth_type(ai, AUTH_OPEN);
+ ai->config.modulation = MOD_CCK;
+
+ if (le16_to_cpu(cap_rid.len) >= sizeof(cap_rid) &&
+ (cap_rid.extSoftCap & cpu_to_le16(1)) &&
+ micsetup(ai) == SUCCESS) {
+ ai->config.opmode |= MODE_MIC;
+ set_bit(FLAG_MIC_CAPABLE, &ai->flags);
+ }
+
+ /* Save off the MAC */
+ for (i = 0; i < ETH_ALEN; i++) {
+ mac[i] = ai->config.macAddr[i];
+ }
+
+ /* Check to see if there are any insmod configured
+ rates to add */
+ if (rates[0]) {
+ memset(ai->config.rates, 0, sizeof(ai->config.rates));
+ for (i = 0; i < 8 && rates[i]; i++) {
+ ai->config.rates[i] = rates[i];
+ }
+ }
+ set_bit (FLAG_COMMIT, &ai->flags);
+
+ return SUCCESS;
+}
+
+
static u16 setup_card(struct airo_info *ai, u8 *mac, int lock)
{
Cmd cmd;
@@ -3864,58 +3926,9 @@ static u16 setup_card(struct airo_info *ai, u8 *mac, int
lock)
if (lock)
up(&ai->sem);
if (ai->config.len == 0) {
- int i;
- tdsRssiRid rssi_rid;
- CapabilityRid cap_rid;
-
- kfree(ai->SSID);
- ai->SSID = NULL;
- // general configuration (read/modify/write)
- status = readConfigRid(ai, lock);
- if (status != SUCCESS) return ERROR;
-
- status = readCapabilityRid(ai, &cap_rid, lock);
- if (status != SUCCESS) return ERROR;
-
- status = PC4500_readrid(ai, RID_RSSI,&rssi_rid,
sizeof(rssi_rid), lock);
- if (status == SUCCESS) {
- if (ai->rssi || (ai->rssi = kmalloc(512, GFP_KERNEL))
!= NULL)
- memcpy(ai->rssi, (u8*)&rssi_rid + 2, 512); /*
Skip RID length member */
- }
- else {
- kfree(ai->rssi);
- ai->rssi = NULL;
- if (cap_rid.softCap & cpu_to_le16(8))
- ai->config.rmode |
[PATCH] rhashtable: avoid -Wrestrict warning on overlapping sprintf output
From: Arnd Bergmann
sprintf() is declared with a restrict keyword to not allow input and
output to point to the same buffer:
lib/test_rhashtable.c: In function 'print_ht':
lib/test_rhashtable.c:504:4: error: 'sprintf' argument 3 overlaps destination
object 'buff' [-Werror=restrict]
504 |sprintf(buff, "%s\nbucket[%d] -> ", buff, i);
|^~~~
lib/test_rhashtable.c:489:7: note: destination object referenced by
'restrict'-qualified argument 1 was declared here
489 | char buff[512] = "";
| ^~~~
Rework this function to remember the last offset instead to
avoid the warning.
Signed-off-by: Arnd Bergmann
---
lib/test_rhashtable.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/lib/test_rhashtable.c b/lib/test_rhashtable.c
index 76c607ee6db5..5a1dd4736b56 100644
--- a/lib/test_rhashtable.c
+++ b/lib/test_rhashtable.c
@@ -487,6 +487,7 @@ static unsigned int __init print_ht(struct rhltable *rhlt)
struct rhashtable *ht;
const struct bucket_table *tbl;
char buff[512] = "";
+ int offset = 0;
unsigned int i, cnt = 0;
ht = &rhlt->ht;
@@ -501,18 +502,18 @@ static unsigned int __init print_ht(struct rhltable *rhlt)
next = !rht_is_a_nulls(pos) ? rht_dereference(pos->next, ht) :
NULL;
if (!rht_is_a_nulls(pos)) {
- sprintf(buff, "%s\nbucket[%d] -> ", buff, i);
+ offset += sprintf(buff + offset, "\nbucket[%d] -> ", i);
}
while (!rht_is_a_nulls(pos)) {
struct rhlist_head *list = container_of(pos, struct
rhlist_head, rhead);
- sprintf(buff, "%s[[", buff);
+ offset += sprintf(buff + offset, "[[");
do {
pos = &list->rhead;
list = rht_dereference(list->next, ht);
p = rht_obj(ht, pos);
- sprintf(buff, "%s val %d (tid=%d)%s", buff,
p->value.id, p->value.tid,
+ offset += sprintf(buff + offset, " val %d
(tid=%d)%s", p->value.id, p->value.tid,
list? ", " : " ");
cnt++;
} while (list);
@@ -521,7 +522,7 @@ static unsigned int __init print_ht(struct rhltable *rhlt)
next = !rht_is_a_nulls(pos) ?
rht_dereference(pos->next, ht) : NULL;
- sprintf(buff, "%s]]%s", buff, !rht_is_a_nulls(pos) ? "
-> " : "");
+ offset += sprintf(buff + offset, "]]%s",
!rht_is_a_nulls(pos) ? " -> " : "");
}
}
printk(KERN_ERR "\n ht: %s\n-\n", buff);
--
2.29.2
[PATCH net-next] wlcore: fix overlapping snprintf arguments in debugfs
From: Arnd Bergmann
gcc complains about undefined behavior in calling snprintf()
with the same buffer as input and output:
drivers/net/wireless/ti/wl18xx/debugfs.c: In function
'diversity_num_of_packets_per_ant_read':
drivers/net/wireless/ti/wl18xx/../wlcore/debugfs.h:86:3: error: 'snprintf'
argument 4 overlaps destination object 'buf' [-Werror=restrict]
86 | snprintf(buf, sizeof(buf), "%s[%d] = %d\n", \
| ^~
87 | buf, i, stats->sub.name[i]); \
| ~~~
drivers/net/wireless/ti/wl18xx/debugfs.c:24:2: note: in expansion of macro
'DEBUGFS_FWSTATS_FILE_ARRAY'
24 | DEBUGFS_FWSTATS_FILE_ARRAY(a, b, c, wl18xx_acx_statistics)
| ^~
drivers/net/wireless/ti/wl18xx/debugfs.c:159:1: note: in expansion of macro
'WL18XX_DEBUGFS_FWSTATS_FILE_ARRAY'
159 | WL18XX_DEBUGFS_FWSTATS_FILE_ARRAY(diversity, num_of_packets_per_ant,
There are probably other ways of handling the debugfs file, without
using on-stack buffers, but a simple workaround here is to remember the
current position in the buffer and just keep printing in there.
Fixes: bcca1bbdd412 ("wlcore: add debugfs macro to help print fw statistics
arrays")
Signed-off-by: Arnd Bergmann
---
drivers/net/wireless/ti/wlcore/boot.c| 13 -
drivers/net/wireless/ti/wlcore/debugfs.h | 7 ---
2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ti/wlcore/boot.c
b/drivers/net/wireless/ti/wlcore/boot.c
index e14d88e558f0..85abd0a2d1c9 100644
--- a/drivers/net/wireless/ti/wlcore/boot.c
+++ b/drivers/net/wireless/ti/wlcore/boot.c
@@ -72,6 +72,7 @@ static int wlcore_validate_fw_ver(struct wl1271 *wl)
unsigned int *min_ver = (wl->fw_type == WL12XX_FW_TYPE_MULTI) ?
wl->min_mr_fw_ver : wl->min_sr_fw_ver;
char min_fw_str[32] = "";
+ int off = 0;
int i;
/* the chip must be exactly equal */
@@ -105,13 +106,15 @@ static int wlcore_validate_fw_ver(struct wl1271 *wl)
return 0;
fail:
- for (i = 0; i < NUM_FW_VER; i++)
+ for (i = 0; i < NUM_FW_VER && off < sizeof(min_fw_str); i++)
if (min_ver[i] == WLCORE_FW_VER_IGNORE)
- snprintf(min_fw_str, sizeof(min_fw_str),
- "%s*.", min_fw_str);
+ off += snprintf(min_fw_str + off,
+ sizeof(min_fw_str) - off,
+ "*.");
else
- snprintf(min_fw_str, sizeof(min_fw_str),
- "%s%u.", min_fw_str, min_ver[i]);
+ off += snprintf(min_fw_str + off,
+ sizeof(min_fw_str) - off,
+ "%u.", min_ver[i]);
wl1271_error("Your WiFi FW version (%u.%u.%u.%u.%u) is invalid.\n"
"Please use at least FW %s\n"
diff --git a/drivers/net/wireless/ti/wlcore/debugfs.h
b/drivers/net/wireless/ti/wlcore/debugfs.h
index b143293e694f..715edfa5f89f 100644
--- a/drivers/net/wireless/ti/wlcore/debugfs.h
+++ b/drivers/net/wireless/ti/wlcore/debugfs.h
@@ -78,13 +78,14 @@ static ssize_t sub## _ ##name## _read(struct file *file,
\
struct wl1271 *wl = file->private_data; \
struct struct_type *stats = wl->stats.fw_stats; \
char buf[DEBUGFS_FORMAT_BUFFER_SIZE] = ""; \
+ int pos = 0;\
int i; \
\
wl1271_debugfs_update_stats(wl);\
\
- for (i = 0; i < len; i++) \
- snprintf(buf, sizeof(buf), "%s[%d] = %d\n", \
-buf, i, stats->sub.name[i]); \
+ for (i = 0; i < len && pos < sizeof(buf); i++) \
+ pos += snprintf(buf + pos, sizeof(buf), \
+"[%d] = %d\n", i, stats->sub.name[i]); \
\
return wl1271_format_buffer(userbuf, count, ppos, "%s", buf); \
} \
--
2.29.2
[PATCH net-next] hinic: avoid gcc -Wrestrict warning
From: Arnd Bergmann
With extra warnings enabled, gcc complains that snprintf should not
take the same buffer as source and destination:
drivers/net/ethernet/huawei/hinic/hinic_ethtool.c: In function
'hinic_set_settings_to_hw':
drivers/net/ethernet/huawei/hinic/hinic_ethtool.c:480:9: error: 'snprintf'
argument 4 overlaps destination object 'set_link_str' [-Werror=restrict]
480 | err = snprintf(set_link_str, SET_LINK_STR_MAX_LEN,
| ^~~~
481 | "%sspeed %d ", set_link_str, speed);
| ~~~
drivers/net/ethernet/huawei/hinic/hinic_ethtool.c:464:7: note: destination
object referenced by 'restrict'-qualified argument 1 was declared here
464 | char set_link_str[SET_LINK_STR_MAX_LEN] = {0};
Rewrite this to remember the offset of the previous printf output
instead.
Signed-off-by: Arnd Bergmann
---
drivers/net/ethernet/huawei/hinic/hinic_ethtool.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/huawei/hinic/hinic_ethtool.c
b/drivers/net/ethernet/huawei/hinic/hinic_ethtool.c
index c340d9acba80..74aefc8fc4d8 100644
--- a/drivers/net/ethernet/huawei/hinic/hinic_ethtool.c
+++ b/drivers/net/ethernet/huawei/hinic/hinic_ethtool.c
@@ -464,7 +464,7 @@ static int hinic_set_settings_to_hw(struct hinic_dev
*nic_dev,
char set_link_str[SET_LINK_STR_MAX_LEN] = {0};
struct net_device *netdev = nic_dev->netdev;
enum nic_speed_level speed_level = 0;
- int err;
+ int err, off;
err = snprintf(set_link_str, SET_LINK_STR_MAX_LEN, "%s",
(set_settings & HILINK_LINK_SET_AUTONEG) ?
@@ -475,10 +475,11 @@ static int hinic_set_settings_to_hw(struct hinic_dev
*nic_dev,
return -EFAULT;
}
+ off = err;
if (set_settings & HILINK_LINK_SET_SPEED) {
speed_level = hinic_ethtool_to_hw_speed_level(speed);
- err = snprintf(set_link_str, SET_LINK_STR_MAX_LEN,
- "%sspeed %d ", set_link_str, speed);
+ err = snprintf(set_link_str + off, SET_LINK_STR_MAX_LEN - off,
+ "speed %d ", speed);
if (err <= 0 || err >= SET_LINK_STR_MAX_LEN) {
netif_err(nic_dev, drv, netdev, "Failed to snprintf
link speed, function return(%d) and dest_len(%d)\n",
err, SET_LINK_STR_MAX_LEN);
--
2.29.2
[PATCH net-next] octeontx2: fix -Wnonnull warning
From: Arnd Bergmann
When compile testing this driver on a platform on which probe() is
known to fail at compile time, gcc warns about the cgx_lmactype_string[]
array being uninitialized:
In function 'strncpy',
inlined from 'link_status_user_format' at
/git/arm-soc/drivers/net/ethernet/marvell/octeontx2/af/cgx.c:838:2,
inlined from 'cgx_link_change_handler' at
/git/arm-soc/drivers/net/ethernet/marvell/octeontx2/af/cgx.c:853:2:
include/linux/fortify-string.h:27:30: error: argument 2 null where non-null
expected [-Werror=nonnull]
27 | #define __underlying_strncpy __builtin_strncpy
Address this by turning the runtime initialization into a fixed array,
which should also produce better code.
Signed-off-by: Arnd Bergmann
---
.../net/ethernet/marvell/octeontx2/af/cgx.c | 60 +--
1 file changed, 28 insertions(+), 32 deletions(-)
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
b/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
index 9caa375d01b1..ea5a033a1d0b 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
@@ -30,10 +30,35 @@
static LIST_HEAD(cgx_list);
/* Convert firmware speed encoding to user format(Mbps) */
-static u32 cgx_speed_mbps[CGX_LINK_SPEED_MAX];
+static const u32 cgx_speed_mbps[CGX_LINK_SPEED_MAX] = {
+ [CGX_LINK_NONE] = 0,
+ [CGX_LINK_10M] = 10,
+ [CGX_LINK_100M] = 100,
+ [CGX_LINK_1G] = 1000,
+ [CGX_LINK_2HG] = 2500,
+ [CGX_LINK_5G] = 5000,
+ [CGX_LINK_10G] = 1,
+ [CGX_LINK_20G] = 2,
+ [CGX_LINK_25G] = 25000,
+ [CGX_LINK_40G] = 4,
+ [CGX_LINK_50G] = 5,
+ [CGX_LINK_80G] = 8,
+ [CGX_LINK_100G] = 10,
+};
/* Convert firmware lmac type encoding to string */
-static char *cgx_lmactype_string[LMAC_MODE_MAX];
+static const char *cgx_lmactype_string[LMAC_MODE_MAX] = {
+ [LMAC_MODE_SGMII] = "SGMII",
+ [LMAC_MODE_XAUI] = "XAUI",
+ [LMAC_MODE_RXAUI] = "RXAUI",
+ [LMAC_MODE_10G_R] = "10G_R",
+ [LMAC_MODE_40G_R] = "40G_R",
+ [LMAC_MODE_QSGMII] = "QSGMII",
+ [LMAC_MODE_25G_R] = "25G_R",
+ [LMAC_MODE_50G_R] = "50G_R",
+ [LMAC_MODE_100G_R] = "100G_R",
+ [LMAC_MODE_USXGMII] = "USXGMII",
+};
/* CGX PHY management internal APIs */
static int cgx_fwi_link_change(struct cgx *cgx, int lmac_id, bool en);
@@ -657,34 +682,6 @@ int cgx_fwi_cmd_generic(u64 req, u64 *resp, struct cgx
*cgx, int lmac_id)
return err;
}
-static inline void cgx_link_usertable_init(void)
-{
- cgx_speed_mbps[CGX_LINK_NONE] = 0;
- cgx_speed_mbps[CGX_LINK_10M] = 10;
- cgx_speed_mbps[CGX_LINK_100M] = 100;
- cgx_speed_mbps[CGX_LINK_1G] = 1000;
- cgx_speed_mbps[CGX_LINK_2HG] = 2500;
- cgx_speed_mbps[CGX_LINK_5G] = 5000;
- cgx_speed_mbps[CGX_LINK_10G] = 1;
- cgx_speed_mbps[CGX_LINK_20G] = 2;
- cgx_speed_mbps[CGX_LINK_25G] = 25000;
- cgx_speed_mbps[CGX_LINK_40G] = 4;
- cgx_speed_mbps[CGX_LINK_50G] = 5;
- cgx_speed_mbps[CGX_LINK_80G] = 8;
- cgx_speed_mbps[CGX_LINK_100G] = 10;
-
- cgx_lmactype_string[LMAC_MODE_SGMII] = "SGMII";
- cgx_lmactype_string[LMAC_MODE_XAUI] = "XAUI";
- cgx_lmactype_string[LMAC_MODE_RXAUI] = "RXAUI";
- cgx_lmactype_string[LMAC_MODE_10G_R] = "10G_R";
- cgx_lmactype_string[LMAC_MODE_40G_R] = "40G_R";
- cgx_lmactype_string[LMAC_MODE_QSGMII] = "QSGMII";
- cgx_lmactype_string[LMAC_MODE_25G_R] = "25G_R";
- cgx_lmactype_string[LMAC_MODE_50G_R] = "50G_R";
- cgx_lmactype_string[LMAC_MODE_100G_R] = "100G_R";
- cgx_lmactype_string[LMAC_MODE_USXGMII] = "USXGMII";
-}
-
static int cgx_link_usertable_index_map(int speed)
{
switch (speed) {
@@ -826,7 +823,7 @@ static inline void link_status_user_format(u64 lstat,
struct cgx_link_user_info *linfo,
struct cgx *cgx, u8 lmac_id)
{
- char *lmac_string;
+ const char *lmac_string;
linfo->link_up = FIELD_GET(RESP_LINKSTAT_UP, lstat);
linfo->full_duplex = FIELD_GET(RESP_LINKSTAT_FDUPLEX, lstat);
@@ -1375,7 +1372,6 @@ static int cgx_probe(struct pci_dev *pdev, const struct
pci_device_id *id)
list_add(&cgx->cgx_list, &cgx_list);
- cgx_link_usertable_init();
cgx_populate_features(cgx);
--
2.29.2
[RFC net] net: skbuff: fix stack variable out of bounds access
From: Arnd Bergmann
gcc-11 warns that the TS_SKB_CB(&state)) cast in skb_find_text()
leads to an out-of-bounds access in skb_prepare_seq_read() after
the addition of a new struct member made skb_seq_state longer
than ts_state:
net/core/skbuff.c: In function ‘skb_find_text’:
net/core/skbuff.c:3498:26: error: array subscript ‘struct skb_seq_state[0]’ is
partly outside array bounds of ‘struct ts_state[1]’ [-Werror=array-bounds]
3498 | st->lower_offset = from;
| ~^~
net/core/skbuff.c:3659:25: note: while referencing ‘state’
3659 | struct ts_state state;
| ^
The warning is currently disabled globally, but I found this
instance during experimental build testing, and it seems
legitimate.
Make the textsearch buffer longer and add a compile-time check to
ensure the two remain the same length.
Fixes: 97550f6fa592 ("net: compound page support in skb_seq_read")
Signed-off-by: Arnd Bergmann
---
include/linux/textsearch.h | 2 +-
net/core/skbuff.c | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/linux/textsearch.h b/include/linux/textsearch.h
index 13770cfe33ad..6673e4d4ac2e 100644
--- a/include/linux/textsearch.h
+++ b/include/linux/textsearch.h
@@ -23,7 +23,7 @@ struct ts_config;
struct ts_state
{
unsigned intoffset;
- charcb[40];
+ charcb[48];
};
/**
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 545a472273a5..dd10d4c5f4bf 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3633,6 +3633,7 @@ static unsigned int skb_ts_get_next_block(unsigned int
offset, const u8 **text,
struct ts_config *conf,
struct ts_state *state)
{
+ BUILD_BUG_ON(sizeof(struct skb_seq_state) > sizeof(state->cb));
return skb_seq_read(offset, text, TS_SKB_CB(state));
}
--
2.29.2
Re: [PATCH 02/11] x86: tboot: avoid Wstringop-overread-warning
On Mon, Mar 22, 2021 at 11:09 PM Martin Sebor wrote:
> On 3/22/21 2:29 PM, Ingo Molnar wrote:
> > * Arnd Bergmann wrote:
> >
> > I.e. the real workaround might be to turn off the
> > -Wstringop-overread-warning,
> > until GCC-11 gets fixed?
>
> In GCC 10 -Wstringop-overread is a subset of -Wstringop-overflow.
> GCC 11 breaks it out as a separate warning to make it easier to
> control. Both warnings have caught some real bugs but they both
> have a nonzero rate of false positives. Other than bug reports
> we don't have enough data to say what their S/N ratio might be
> but my sense is that it's fairly high in general.
>
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=wstringop-overread
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=wstringop-overflow
Unfortunately, stringop-overflow is one of the warnings that is
completely disabled in the kernel at the moment, rather than
enabled at one of the user-selectable higher warning levels.
I have a patch series to change that and to pull some of these
into the lower W=1 or W=2 levels or even enable them by default.
To do this though, I first need to ensure that the actual output
is empty for the normal builds. I added -Wstringop-overflow to
the list of warnings I wanted to address because it is a new
warning and only has a dozen or so occurrences throughout the
kernel.
> In GCC 11, all access warnings expect objects to be either declared
> or allocated. Pointers with constant values are taken to point to
> nothing valid (as Arnd mentioned above, this is to detect invalid
> accesses to members of structs at address zero).
>
> One possible solution to the known address problem is to extend GCC
> attributes address and io that pin an object to a hardwired address
> to all targets (at the moment they're supported on just one or two
> targets). I'm not sure this can still happen before GCC 11 releases
> sometime in April or May.
>
> Until then, another workaround is to convert the fixed address to
> a volatile pointer before using it for the access, along the lines
> below. It should have only a negligible effect on efficiency.
>
> diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
> index 4c09ba110204..76326b906010 100644
> --- a/arch/x86/kernel/tboot.c
> +++ b/arch/x86/kernel/tboot.c
> @@ -67,7 +67,9 @@ void __init tboot_probe(void)
> /* Map and check for tboot UUID. */
> set_fixmap(FIX_TBOOT_BASE, boot_params.tboot_addr);
> tboot = (struct tboot *)fix_to_virt(FIX_TBOOT_BASE);
> - if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) {
> + if (memcmp(&tboot_uuid,
> + (*(struct tboot* volatile *)(&tboot))->uuid,
> + sizeof(tboot->uuid))) {
> pr_warn("tboot at 0x%llx is invalid\n",
I think a stray 'volatile' would raise too many red flags here, but
I checked that the RELOC_HIDE() macro has the same effect, e.g.
@@ -66,7 +67,8 @@ void __init tboot_probe(void)
/* Map and check for tboot UUID. */
set_fixmap(FIX_TBOOT_BASE, boot_params.tboot_addr);
- tboot = (struct tboot *)fix_to_virt(FIX_TBOOT_BASE);
+ /* RELOC_HIDE to prevent gcc warnings about NULL pointer */
+ tboot = RELOC_HIDE(NULL, fix_to_virt(FIX_TBOOT_BASE));
if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) {
pr_warn("tboot at 0x%llx is invalid\n", boot_params.tboot_addr);
tboot = NULL;
Arnd
[PATCH] bpf: avoid old-style declaration warnings
From: Arnd Bergmann
gcc -Wextra wants type modifiers in the normal order:
kernel/bpf/bpf_lsm.c:70:1: error: 'static' is not at beginning of declaration
[-Werror=old-style-declaration]
70 | const static struct bpf_func_proto bpf_bprm_opts_set_proto = {
| ^
kernel/bpf/bpf_lsm.c:91:1: error: 'static' is not at beginning of declaration
[-Werror=old-style-declaration]
91 | const static struct bpf_func_proto bpf_ima_inode_hash_proto = {
| ^
Fixes: 3f6719c7b62f ("bpf: Add bpf_bprm_opts_set helper")
Fixes: 27672f0d280a ("bpf: Add a BPF helper for getting the IMA hash of an
inode")
Signed-off-by: Arnd Bergmann
---
kernel/bpf/bpf_lsm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index 1622a44d1617..75b1c678d558 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -67,7 +67,7 @@ BPF_CALL_2(bpf_bprm_opts_set, struct linux_binprm *, bprm,
u64, flags)
BTF_ID_LIST_SINGLE(bpf_bprm_opts_set_btf_ids, struct, linux_binprm)
-const static struct bpf_func_proto bpf_bprm_opts_set_proto = {
+static const struct bpf_func_proto bpf_bprm_opts_set_proto = {
.func = bpf_bprm_opts_set,
.gpl_only = false,
.ret_type = RET_INTEGER,
@@ -88,7 +88,7 @@ static bool bpf_ima_inode_hash_allowed(const struct bpf_prog
*prog)
BTF_ID_LIST_SINGLE(bpf_ima_inode_hash_btf_ids, struct, inode)
-const static struct bpf_func_proto bpf_ima_inode_hash_proto = {
+static const struct bpf_func_proto bpf_ima_inode_hash_proto = {
.func = bpf_ima_inode_hash,
.gpl_only = false,
.ret_type = RET_INTEGER,
--
2.29.2
[PATCH net-next] iwlwifi: fix old-style-declaration warning
From: Arnd Bergmann
The correct order is 'static const', not 'const static', as seen from
make W=1:
drivers/net/wireless/intel/iwlwifi/mvm/rfi.c:14:1: error: 'static' is not at
beginning of declaration [-Werror=old-style-declaration]
Fixes: 21254908cbe9 ("iwlwifi: mvm: add RFI-M support")
Signed-off-by: Arnd Bergmann
---
drivers/net/wireless/intel/iwlwifi/mvm/rfi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/rfi.c
b/drivers/net/wireless/intel/iwlwifi/mvm/rfi.c
index 873919048143..4d5a99cbcc9d 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/rfi.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/rfi.c
@@ -11,7 +11,7 @@
* DDR needs frequency in units of 16.666MHz, so provide FW with the
* frequency values in the adjusted format.
*/
-const static struct iwl_rfi_lut_entry iwl_rfi_table[IWL_RFI_LUT_SIZE] = {
+static const struct iwl_rfi_lut_entry iwl_rfi_table[IWL_RFI_LUT_SIZE] = {
/* LPDDR4 */
/* frequency 3733MHz */
--
2.29.2
Re: [PATCH 02/11] x86: tboot: avoid Wstringop-overread-warning
On Mon, Mar 22, 2021 at 9:29 PM Ingo Molnar wrote: > * Arnd Bergmann wrote: > > From: Arnd Bergmann > This is indeed rather ugly - and the other patch that removes a debug > check seems counterproductive as well. > > Do we know how many genuine bugs -Wstringop-overread-warning has > caught or is about to catch? > > I.e. the real workaround might be to turn off the -Wstringop-overread-warning, > until GCC-11 gets fixed? See the [PATCH 0/11] message. The last two patches in the series are for code that I suspect may be broken, the others are basically all false positives. As gcc-11 is not released yet, I don't think we have to apply any of the patches or disable the warning at the moment, but I posted all the patches to get a better understanding on which of them should be addressed in the kernel vs gcc. Arnd
[PATCH] bluetooth: fix set_ecdh_privkey() prototype
From: Arnd Bergmann
gcc-11 points out that the declaration does not match the definition:
net/bluetooth/ecdh_helper.c:122:55: error: argument 2 of type ‘const u8[32]’
{aka ‘const unsigned char[32]’} with mismatched bound [-Werror=array-parameter=]
122 | int set_ecdh_privkey(struct crypto_kpp *tfm, const u8 private_key[32])
| ~^~~
In file included from net/bluetooth/ecdh_helper.c:23:
net/bluetooth/ecdh_helper.h:28:56: note: previously declared as ‘const u8 *’
{aka ‘const unsigned char *’}
28 | int set_ecdh_privkey(struct crypto_kpp *tfm, const u8 *private_key);
| ~~^~~
Change the declaration to contain the size of the array, rather than
just a pointer.
Signed-off-by: Arnd Bergmann
---
net/bluetooth/ecdh_helper.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/bluetooth/ecdh_helper.h b/net/bluetooth/ecdh_helper.h
index a6f8d03d4aaf..830723971cf8 100644
--- a/net/bluetooth/ecdh_helper.h
+++ b/net/bluetooth/ecdh_helper.h
@@ -25,6 +25,6 @@
int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 pair_public_key[64],
u8 secret[32]);
-int set_ecdh_privkey(struct crypto_kpp *tfm, const u8 *private_key);
+int set_ecdh_privkey(struct crypto_kpp *tfm, const u8 private_key[32]);
int generate_ecdh_public_key(struct crypto_kpp *tfm, u8 public_key[64]);
int generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64]);
--
2.29.2
[PATCH] isdn: capi: fix mismatched prototypes
From: Arnd Bergmann
gcc-11 complains about a prototype declaration that is different
from the function definition:
drivers/isdn/capi/kcapi.c:724:44: error: argument 2 of type ‘u8 *’ {aka
‘unsigned char *’} declared as a pointer [-Werror=array-parameter=]
724 | u16 capi20_get_manufacturer(u32 contr, u8 *buf)
|^~~
In file included from drivers/isdn/capi/kcapi.c:13:
drivers/isdn/capi/kcapi.h:62:43: note: previously declared as an array ‘u8[64]’
{aka ‘unsigned char[64]’}
62 | u16 capi20_get_manufacturer(u32 contr, u8 buf[CAPI_MANUFACTURER_LEN]);
|~~~^~
drivers/isdn/capi/kcapi.c:790:38: error: argument 2 of type ‘u8 *’ {aka
‘unsigned char *’} declared as a pointer [-Werror=array-parameter=]
790 | u16 capi20_get_serial(u32 contr, u8 *serial)
| ^~
In file included from drivers/isdn/capi/kcapi.c:13:
drivers/isdn/capi/kcapi.h:64:37: note: previously declared as an array ‘u8[8]’
{aka ‘unsigned char[8]’}
64 | u16 capi20_get_serial(u32 contr, u8 serial[CAPI_SERIAL_LEN]);
| ~~~^~~
Change the definition to make them match.
Signed-off-by: Arnd Bergmann
---
drivers/isdn/capi/kcapi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/isdn/capi/kcapi.c b/drivers/isdn/capi/kcapi.c
index 7168778fbbe1..cb0afe897162 100644
--- a/drivers/isdn/capi/kcapi.c
+++ b/drivers/isdn/capi/kcapi.c
@@ -721,7 +721,7 @@ u16 capi20_put_message(struct capi20_appl *ap, struct
sk_buff *skb)
* Return value: CAPI result code
*/
-u16 capi20_get_manufacturer(u32 contr, u8 *buf)
+u16 capi20_get_manufacturer(u32 contr, u8 buf[CAPI_MANUFACTURER_LEN])
{
struct capi_ctr *ctr;
u16 ret;
@@ -787,7 +787,7 @@ u16 capi20_get_version(u32 contr, struct capi_version *verp)
* Return value: CAPI result code
*/
-u16 capi20_get_serial(u32 contr, u8 *serial)
+u16 capi20_get_serial(u32 contr, u8 serial[CAPI_SERIAL_LEN])
{
struct capi_ctr *ctr;
u16 ret;
--
2.29.2
[PATCH 09/11] scsi: lpfc: fix gcc -Wstringop-overread warning
From: Arnd Bergmann
gcc-11 warns about an strnlen with a length larger than the size of the
passed buffer:
drivers/scsi/lpfc/lpfc_attr.c: In function 'lpfc_nvme_info_show':
drivers/scsi/lpfc/lpfc_attr.c:518:25: error: 'strnlen' specified bound 4095
exceeds source size 24 [-Werror=stringop-overread]
518 | strnlen(LPFC_NVME_INFO_MORE_STR, PAGE_SIZE - 1)
| ^~~
In this case, the code is entirely valid, as the string is properly
terminated, and the size argument is only there out of extra caution
in case it exceeds a page.
This cannot really happen here, so just simplify it to a sizeof().
Fixes: afff0d2321ea ("scsi: lpfc: Add Buffer overflow check, when nvme_info
larger than PAGE_SIZE")
Signed-off-by: Arnd Bergmann
---
drivers/scsi/lpfc/lpfc_attr.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/lpfc/lpfc_attr.c b/drivers/scsi/lpfc/lpfc_attr.c
index bdd9a29f4201..f6d886f9dfb3 100644
--- a/drivers/scsi/lpfc/lpfc_attr.c
+++ b/drivers/scsi/lpfc/lpfc_attr.c
@@ -512,11 +512,9 @@ lpfc_nvme_info_show(struct device *dev, struct
device_attribute *attr,
"6314 Catching potential buffer "
"overflow > PAGE_SIZE = %lu bytes\n",
PAGE_SIZE);
- strlcpy(buf + PAGE_SIZE - 1 -
- strnlen(LPFC_NVME_INFO_MORE_STR, PAGE_SIZE - 1),
+ strlcpy(buf + PAGE_SIZE - 1 - sizeof(LPFC_NVME_INFO_MORE_STR),
LPFC_NVME_INFO_MORE_STR,
- strnlen(LPFC_NVME_INFO_MORE_STR, PAGE_SIZE - 1)
- + 1);
+ sizeof(LPFC_NVME_INFO_MORE_STR) + 1);
}
return len;
--
2.29.2
[PATCH 11/11] [RFC] drm/i915/dp: fix array overflow warning
From: Arnd Bergmann
gcc-11 warns that intel_dp_check_mst_status() has a local array of
fourteen bytes and passes the last four bytes into a function that
expects a six-byte array:
drivers/gpu/drm/i915/display/intel_dp.c: In function
‘intel_dp_check_mst_status’:
drivers/gpu/drm/i915/display/intel_dp.c:4556:22: error: ‘drm_dp_channel_eq_ok’
reading 6 bytes from a region of size 4 [-Werror=stringop-overread]
4556 | !drm_dp_channel_eq_ok(&esi[10],
intel_dp->lane_count)) {
|
^~~~
drivers/gpu/drm/i915/display/intel_dp.c:4556:22: note: referencing argument 1
of type ‘const u8 *’ {aka ‘const unsigned char *’}
In file included from drivers/gpu/drm/i915/display/intel_dp.c:38:
include/drm/drm_dp_helper.h:1459:6: note: in a call to function
‘drm_dp_channel_eq_ok’
1459 | bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
| ^~~~
Clearly something is wrong here, but I can't quite figure out what.
Changing the array size to 16 bytes avoids the warning, but is
probably the wrong solution here.
Signed-off-by: Arnd Bergmann
---
drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index 8c12d5375607..830e2515f119 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -65,7 +65,7 @@
#include "intel_vdsc.h"
#include "intel_vrr.h"
-#define DP_DPRX_ESI_LEN 14
+#define DP_DPRX_ESI_LEN 16
/* DP DSC throughput values used for slice count calculations KPixels/s */
#define DP_DSC_PEAK_PIXEL_RATE 272
--
2.29.2
[PATCH 02/11] x86: tboot: avoid Wstringop-overread-warning
From: Arnd Bergmann
gcc-11 warns about using string operations on pointers that are
defined at compile time as offsets from a NULL pointer. Unfortunately
that also happens on the result of fix_to_virt(), which is a
compile-time constant for a constantn input:
arch/x86/kernel/tboot.c: In function 'tboot_probe':
arch/x86/kernel/tboot.c:70:13: error: '__builtin_memcmp_eq' specified bound 16
exceeds source size 0 [-Werror=stringop-overread]
70 | if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) {
| ^~
I hope this can get addressed in gcc-11 before the release.
As a workaround, split up the tboot_probe() function in two halves
to separate the pointer generation from the usage. This is a bit
ugly, and hopefully gcc understands that the code is actually correct
before it learns to peek into the noinline function.
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
Signed-off-by: Arnd Bergmann
---
arch/x86/kernel/tboot.c | 44 -
1 file changed, 26 insertions(+), 18 deletions(-)
diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
index 4c09ba110204..f9af561c3cd4 100644
--- a/arch/x86/kernel/tboot.c
+++ b/arch/x86/kernel/tboot.c
@@ -49,6 +49,30 @@ bool tboot_enabled(void)
return tboot != NULL;
}
+/* noinline to prevent gcc from warning about dereferencing constant fixaddr */
+static noinline __init bool check_tboot_version(void)
+{
+ if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) {
+ pr_warn("tboot at 0x%llx is invalid\n", boot_params.tboot_addr);
+ return false;
+ }
+
+ if (tboot->version < 5) {
+ pr_warn("tboot version is invalid: %u\n", tboot->version);
+ return false;
+ }
+
+ pr_info("found shared page at phys addr 0x%llx:\n",
+ boot_params.tboot_addr);
+ pr_debug("version: %d\n", tboot->version);
+ pr_debug("log_addr: 0x%08x\n", tboot->log_addr);
+ pr_debug("shutdown_entry: 0x%x\n", tboot->shutdown_entry);
+ pr_debug("tboot_base: 0x%08x\n", tboot->tboot_base);
+ pr_debug("tboot_size: 0x%x\n", tboot->tboot_size);
+
+ return true;
+}
+
void __init tboot_probe(void)
{
/* Look for valid page-aligned address for shared page. */
@@ -66,25 +90,9 @@ void __init tboot_probe(void)
/* Map and check for tboot UUID. */
set_fixmap(FIX_TBOOT_BASE, boot_params.tboot_addr);
- tboot = (struct tboot *)fix_to_virt(FIX_TBOOT_BASE);
- if (memcmp(&tboot_uuid, &tboot->uuid, sizeof(tboot->uuid))) {
- pr_warn("tboot at 0x%llx is invalid\n", boot_params.tboot_addr);
+ tboot = (void *)fix_to_virt(FIX_TBOOT_BASE);
+ if (!check_tboot_version())
tboot = NULL;
- return;
- }
- if (tboot->version < 5) {
- pr_warn("tboot version is invalid: %u\n", tboot->version);
- tboot = NULL;
- return;
- }
-
- pr_info("found shared page at phys addr 0x%llx:\n",
- boot_params.tboot_addr);
- pr_debug("version: %d\n", tboot->version);
- pr_debug("log_addr: 0x%08x\n", tboot->log_addr);
- pr_debug("shutdown_entry: 0x%x\n", tboot->shutdown_entry);
- pr_debug("tboot_base: 0x%08x\n", tboot->tboot_base);
- pr_debug("tboot_size: 0x%x\n", tboot->tboot_size);
}
static pgd_t *tboot_pg_dir;
--
2.29.2
[PATCH 10/11] drm/i915: avoid stringop-overread warning on pri_latency
From: Arnd Bergmann
gcc-11 warns about what appears to be an out-of-range array access:
In function ‘snb_wm_latency_quirk’,
inlined from ‘ilk_setup_wm_latency’ at
drivers/gpu/drm/i915/intel_pm.c:3108:3:
drivers/gpu/drm/i915/intel_pm.c:3057:9: error: ‘intel_print_wm_latency’ reading
16 bytes from a region of size 10 [-Werror=stringop-overread]
3057 | intel_print_wm_latency(dev_priv, "Primary",
dev_priv->wm.pri_latency);
|
^
drivers/gpu/drm/i915/intel_pm.c: In function ‘ilk_setup_wm_latency’:
drivers/gpu/drm/i915/intel_pm.c:3057:9: note: referencing argument 3 of type
‘const u16 *’ {aka ‘const short unsigned int *’}
drivers/gpu/drm/i915/intel_pm.c:2994:13: note: in a call to function
‘intel_print_wm_latency’
2994 | static void intel_print_wm_latency(struct drm_i915_private *dev_priv,
| ^~
My guess is that this code is actually safe because the size of the
array depends on the hardware generation, and the function checks for
that, but at the same time I would not expect the compiler to work it
out correctly, and the code seems a little fragile with regards to
future changes. Simply increasing the size of the array should help.
Signed-off-by: Arnd Bergmann
---
drivers/gpu/drm/i915/i915_drv.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 26d69d06aa6d..3567602e0a35 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1095,11 +1095,11 @@ struct drm_i915_private {
* in 0.5us units for WM1+.
*/
/* primary */
- u16 pri_latency[5];
+ u16 pri_latency[8];
/* sprite */
- u16 spr_latency[5];
+ u16 spr_latency[8];
/* cursor */
- u16 cur_latency[5];
+ u16 cur_latency[8];
/*
* Raw watermark memory latency values
* for SKL for all 8 levels
--
2.29.2
[PATCH 08/11] atmel: avoid gcc -Wstringop-overflow warning
From: Arnd Bergmann
gcc-11 notices that the fields as defined in the ass_req_format
structure do not match the actual use of that structure:
cc1: error: writing 4 bytes into a region of size between 18446744073709551613
and 2 [-Werror=stringop-overflow=]
drivers/net/wireless/atmel/atmel.c:2884:20: note: at offset [4, 6] into
destination object ‘ap’ of size 6
2884 | u8 ap[ETH_ALEN]; /* nothing after here directly
accessible */
|^~
drivers/net/wireless/atmel/atmel.c:2885:20: note: at offset [4, 6] into
destination object ‘ssid_el_id’ of size 1
2885 | u8 ssid_el_id;
|^~
This is expected here because the actual structure layout is variable.
As the code does not actually access the individual fields, replace
them with a comment and fixed-length array so prevent gcc from
complaining about it.
Signed-off-by: Arnd Bergmann
---
drivers/net/wireless/atmel/atmel.c | 25 +++--
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/net/wireless/atmel/atmel.c
b/drivers/net/wireless/atmel/atmel.c
index 707fe66727f8..ff9152d600e1 100644
--- a/drivers/net/wireless/atmel/atmel.c
+++ b/drivers/net/wireless/atmel/atmel.c
@@ -2881,13 +2881,18 @@ static void send_association_request(struct
atmel_private *priv, int is_reassoc)
struct ass_req_format {
__le16 capability;
__le16 listen_interval;
- u8 ap[ETH_ALEN]; /* nothing after here directly accessible */
- u8 ssid_el_id;
- u8 ssid_len;
- u8 ssid[MAX_SSID_LENGTH];
- u8 sup_rates_el_id;
- u8 sup_rates_len;
- u8 rates[4];
+ u8 ssid_el[ETH_ALEN + 2 + MAX_SSID_LENGTH + 2 + 4];
+ /*
+* nothing after here directly accessible:
+*
+* u8 ap[ETH_ALEN];
+* u8 ssid_el_id;
+* u8 ssid_len;
+* u8 ssid[MAX_SSID_LENGTH];
+* u8 sup_rates_el_id;
+* u8 sup_rates_len;
+* u8 rates[4];
+*/
} body;
header.frame_control = cpu_to_le16(IEEE80211_FTYPE_MGMT |
@@ -2907,13 +2912,13 @@ static void send_association_request(struct
atmel_private *priv, int is_reassoc)
body.listen_interval = cpu_to_le16(priv->listen_interval *
priv->beacon_period);
+ ssid_el_p = body.ssid_el;
/* current AP address - only in reassoc frame */
if (is_reassoc) {
- memcpy(body.ap, priv->CurrentBSSID, ETH_ALEN);
- ssid_el_p = &body.ssid_el_id;
+ memcpy(ssid_el_p, priv->CurrentBSSID, ETH_ALEN);
+ ssid_el_p += ETH_ALEN;
bodysize = 18 + priv->SSID_size;
} else {
- ssid_el_p = &body.ap[0];
bodysize = 12 + priv->SSID_size;
}
--
2.29.2
[PATCH 03/11] security: commoncap: fix -Wstringop-overread warning
From: Arnd Bergmann gcc-11 introdces a harmless warning for cap_inode_getsecurity: security/commoncap.c: In function ‘cap_inode_getsecurity’: security/commoncap.c:440:33: error: ‘memcpy’ reading 16 bytes from a region of size 0 [-Werror=stringop-overread] 440 | memcpy(&nscap->data, &cap->data, sizeof(__le32) * 2 * VFS_CAP_U32); | ^~ The problem here is that tmpbuf is initialized to NULL, so gcc assumes it is not accessible unless it gets set by vfs_getxattr_alloc(). This is a legitimate warning as far as I can tell, but the code is correct since it correctly handles the error when that function fails. Add a separate NULL check to tell gcc about it as well. Signed-off-by: Arnd Bergmann --- security/commoncap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/commoncap.c b/security/commoncap.c index 28f4d25480df..9a36ed6dd737 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -400,7 +400,7 @@ int cap_inode_getsecurity(struct user_namespace *mnt_userns, &tmpbuf, size, GFP_NOFS); dput(dentry); - if (ret < 0) + if (ret < 0 || !tmpbuf) return ret; fs_ns = inode->i_sb->s_user_ns; -- 2.29.2
[PATCH 06/11] cgroup: fix -Wzero-length-bounds warnings
From: Arnd Bergmann
When cgroups are enabled, but every single subsystem is turned off,
CGROUP_SUBSYS_COUNT is zero, and the cgrp->subsys[] array has no
members.
gcc-11 points out that this leads to an invalid access in any function
that might access this array:
kernel/cgroup/cgroup.c: In function 'cgroup_addrm_files':
kernel/cgroup/cgroup.c:460:58: warning: array subscript '' is outside
the bounds of an interior zero-length array 'struct cgroup_subsys_state *[0]'
[-Wzero-length-bounds]
kernel/cgroup/cgroup.c:460:24: note: in expansion of macro
'rcu_dereference_check'
460 | return rcu_dereference_check(cgrp->subsys[ss->id],
|^
In file included from include/linux/cgroup.h:28,
from kernel/cgroup/cgroup-internal.h:5,
from kernel/cgroup/cgroup.c:31:
include/linux/cgroup-defs.h:422:43: note: while referencing 'subsys'
422 | struct cgroup_subsys_state __rcu *subsys[CGROUP_SUBSYS_COUNT];
I'm not sure what is expected to happen for such a configuration,
presumably these functions are never calls in that case. Adding a
sanity check in each function we get the warning for manages to shut
up the warnings and do nothing instead.
Signed-off-by: Arnd Bergmann
---
I'm grouping this together with the -Wstringop-overread warnings,
since the underlying logic in gcc seems to be the same.
---
kernel/cgroup/cgroup.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 9153b20e5cc6..3477f1dc7872 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -456,7 +456,7 @@ static u16 cgroup_ss_mask(struct cgroup *cgrp)
static struct cgroup_subsys_state *cgroup_css(struct cgroup *cgrp,
struct cgroup_subsys *ss)
{
- if (ss)
+ if (ss && (CGROUP_SUBSYS_COUNT > 0))
return rcu_dereference_check(cgrp->subsys[ss->id],
lockdep_is_held(&cgroup_mutex));
else
@@ -534,6 +534,9 @@ struct cgroup_subsys_state *cgroup_e_css(struct cgroup
*cgrp,
{
struct cgroup_subsys_state *css;
+ if (CGROUP_SUBSYS_COUNT == 0)
+ return NULL;
+
do {
css = cgroup_css(cgrp, ss);
@@ -561,6 +564,9 @@ struct cgroup_subsys_state *cgroup_get_e_css(struct cgroup
*cgrp,
{
struct cgroup_subsys_state *css;
+ if (CGROUP_SUBSYS_COUNT == 0)
+ return NULL;
+
rcu_read_lock();
do {
@@ -630,7 +636,7 @@ struct cgroup_subsys_state *of_css(struct kernfs_open_file
*of)
* the matching css from the cgroup's subsys table is guaranteed to
* be and stay valid until the enclosing operation is complete.
*/
- if (cft->ss)
+ if (cft->ss && CGROUP_SUBSYS_COUNT > 0)
return rcu_dereference_raw(cgrp->subsys[cft->ss->id]);
else
return &cgrp->self;
@@ -2343,6 +2349,9 @@ struct task_struct *cgroup_taskset_next(struct
cgroup_taskset *tset,
struct css_set *cset = tset->cur_cset;
struct task_struct *task = tset->cur_task;
+ if (CGROUP_SUBSYS_COUNT == 0)
+ return NULL;
+
while (&cset->mg_node != tset->csets) {
if (!task)
task = list_first_entry(&cset->mg_tasks,
@@ -4523,7 +4532,7 @@ void css_task_iter_start(struct cgroup_subsys_state *css,
unsigned int flags,
it->ss = css->ss;
it->flags = flags;
- if (it->ss)
+ if (it->ss && CGROUP_SUBSYS_COUNT > 0)
it->cset_pos = &css->cgroup->e_csets[css->ss->id];
else
it->cset_pos = &css->cgroup->cset_links;
--
2.29.2
[PATCH 07/11] ARM: sharpsl_param: work around -Wstringop-overread warning
From: Arnd Bergmann
gcc warns that accessing a pointer based on a numeric constant may
be an offset into a NULL pointer, and would therefore has zero
accessible bytes:
arch/arm/common/sharpsl_param.c: In function ‘sharpsl_save_param’:
arch/arm/common/sharpsl_param.c:43:9: error: ‘memcpy’ reading 64 bytes from a
region of size 0 [-Werror=stringop-overread]
43 | memcpy(&sharpsl_param, param_start(PARAM_BASE), sizeof(struct
sharpsl_param_info));
|
^~
In this particular case, the warning is bogus since this is the actual
pointer, not an offset on a NULL pointer. Add a local variable to shut
up the warning and hope it doesn't come back.
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
Signed-off-by: Arnd Bergmann
---
arch/arm/common/sharpsl_param.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/arm/common/sharpsl_param.c b/arch/arm/common/sharpsl_param.c
index efeb5724d9e9..6237ede2f0c7 100644
--- a/arch/arm/common/sharpsl_param.c
+++ b/arch/arm/common/sharpsl_param.c
@@ -40,7 +40,9 @@ EXPORT_SYMBOL(sharpsl_param);
void sharpsl_save_param(void)
{
- memcpy(&sharpsl_param, param_start(PARAM_BASE), sizeof(struct
sharpsl_param_info));
+ struct sharpsl_param_info *params = param_start(PARAM_BASE);
+
+ memcpy(&sharpsl_param, params, sizeof(*params));
if (sharpsl_param.comadj_keyword != COMADJ_MAGIC)
sharpsl_param.comadj=-1;
--
2.29.2
[PATCH 05/11] qnx: avoid -Wstringop-overread warning
From: Arnd Bergmann
gcc-11 warns that the size of the link name is longer than the di_fname
field:
fs/qnx4/dir.c: In function ‘qnx4_readdir’:
fs/qnx4/dir.c:51:32: error: ‘strnlen’ specified bound 48 exceeds source size 16
[-Werror=stringop-overread]
51 | size = strnlen(de->di_fname, size);
|^~~
In file included from fs/qnx4/qnx4.h:3,
from fs/qnx4/dir.c:16:
include/uapi/linux/qnx4_fs.h:45:25: note: source object declared here
45 | chardi_fname[QNX4_SHORT_NAME_MAX];
The problem here is that we access the same pointer using two different
structure layouts, but gcc determines the object size based on
whatever it encounters first.
Change the strnlen to use the correct field size in each case, and
change the first access to be on the longer field.
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
Signed-off-by: Arnd Bergmann
---
fs/qnx4/dir.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/fs/qnx4/dir.c b/fs/qnx4/dir.c
index a6ee23aadd28..68046450e543 100644
--- a/fs/qnx4/dir.c
+++ b/fs/qnx4/dir.c
@@ -39,21 +39,20 @@ static int qnx4_readdir(struct file *file, struct
dir_context *ctx)
ix = (ctx->pos >> QNX4_DIR_ENTRY_SIZE_BITS) %
QNX4_INODES_PER_BLOCK;
for (; ix < QNX4_INODES_PER_BLOCK; ix++, ctx->pos +=
QNX4_DIR_ENTRY_SIZE) {
offset = ix * QNX4_DIR_ENTRY_SIZE;
- de = (struct qnx4_inode_entry *) (bh->b_data + offset);
- if (!de->di_fname[0])
+ le = (struct qnx4_link_info *)(bh->b_data + offset);
+ de = (struct qnx4_inode_entry *)(bh->b_data + offset);
+ if (!le->dl_fname[0])
continue;
if (!(de->di_status & (QNX4_FILE_USED|QNX4_FILE_LINK)))
continue;
if (!(de->di_status & QNX4_FILE_LINK))
- size = QNX4_SHORT_NAME_MAX;
+ size = strnlen(de->di_fname,
sizeof(de->di_fname));
else
- size = QNX4_NAME_MAX;
- size = strnlen(de->di_fname, size);
+ size = strnlen(le->dl_fname,
sizeof(le->dl_fname));
QNX4DEBUG((KERN_INFO "qnx4_readdir:%.*s\n", size,
de->di_fname));
if (!(de->di_status & QNX4_FILE_LINK))
ino = blknum * QNX4_INODES_PER_BLOCK + ix - 1;
else {
- le = (struct qnx4_link_info*)de;
ino = ( le32_to_cpu(le->dl_inode_blk) - 1 ) *
QNX4_INODES_PER_BLOCK +
le->dl_inode_ndx;
--
2.29.2
[PATCH 04/11] ath11: Wstringop-overread warning
From: Arnd Bergmann
gcc-11 with the kernel address sanitizer prints a warning for this
driver:
In function 'ath11k_peer_assoc_h_vht',
inlined from 'ath11k_peer_assoc_prepare' at
drivers/net/wireless/ath/ath11k/mac.c:1632:2:
drivers/net/wireless/ath/ath11k/mac.c:1164:13: error:
'ath11k_peer_assoc_h_vht_masked' reading 16 bytes from a region of size 4
[-Werror=stringop-overread]
1164 | if (ath11k_peer_assoc_h_vht_masked(vht_mcs_mask))
| ^~~~
drivers/net/wireless/ath/ath11k/mac.c: In function 'ath11k_peer_assoc_prepare':
drivers/net/wireless/ath/ath11k/mac.c:1164:13: note: referencing argument 1 of
type 'const u16 *' {aka 'const short unsigned int *'}
drivers/net/wireless/ath/ath11k/mac.c:969:1: note: in a call to function
'ath11k_peer_assoc_h_vht_masked'
969 | ath11k_peer_assoc_h_vht_masked(const u16
vht_mcs_mask[NL80211_VHT_NSS_MAX])
| ^~
According to analysis from gcc developers, this is a glitch in the
way gcc tracks the size of struct members. This should really get
fixed in gcc, but it's also easy to work around this instance
by changing the function prototype to no include the length of
the array.
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99673
Signed-off-by: Arnd Bergmann
---
drivers/net/wireless/ath/ath11k/mac.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath11k/mac.c
b/drivers/net/wireless/ath/ath11k/mac.c
index b391169576e2..5cb7ed53f3c4 100644
--- a/drivers/net/wireless/ath/ath11k/mac.c
+++ b/drivers/net/wireless/ath/ath11k/mac.c
@@ -966,7 +966,7 @@ ath11k_peer_assoc_h_ht_masked(const u8
ht_mcs_mask[IEEE80211_HT_MCS_MASK_LEN])
}
static bool
-ath11k_peer_assoc_h_vht_masked(const u16 vht_mcs_mask[NL80211_VHT_NSS_MAX])
+ath11k_peer_assoc_h_vht_masked(const u16 vht_mcs_mask[])
{
int nss;
--
2.29.2
[PATCH 01/11] x86: compressed: avoid gcc-11 -Wstringop-overread warning
From: Arnd Bergmann
gcc gets confused by the comparison of a pointer to an integer listeral,
with the assumption that this is an offset from a NULL pointer and that
dereferencing it is invalid:
In file included from arch/x86/boot/compressed/misc.c:18:
In function ‘parse_elf’,
inlined from ‘extract_kernel’ at arch/x86/boot/compressed/misc.c:442:2:
arch/x86/boot/compressed/../string.h:15:23: error: ‘__builtin_memcpy’ reading
64 bytes from a region of size 0 [-Werror=stringop-overread]
15 | #define memcpy(d,s,l) __builtin_memcpy(d,s,l)
| ^~~
arch/x86/boot/compressed/misc.c:283:9: note: in expansion of macro ‘memcpy’
283 | memcpy(&ehdr, output, sizeof(ehdr));
| ^~
I could not find any good workaround for this, but as this is only
a warning for a failure during early boot, removing the line entirely
works around the warning.
This should probably get addressed in gcc instead, before 11.1 gets
released.
Signed-off-by: Arnd Bergmann
---
arch/x86/boot/compressed/misc.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 3a214cc3239f..9ada64e66cb7 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -430,8 +430,6 @@ asmlinkage __visible void *extract_kernel(void *rmode,
memptr heap,
error("Destination address too large");
#endif
#ifndef CONFIG_RELOCATABLE
- if ((unsigned long)output != LOAD_PHYSICAL_ADDR)
- error("Destination address does not match LOAD_PHYSICAL_ADDR");
if (virt_addr != LOAD_PHYSICAL_ADDR)
error("Destination virtual address changed when not
relocatable");
#endif
--
2.29.2
[PATCH 00/11] treewide: address gcc-11 -Wstringop-overread warnings
From: Arnd Bergmann The coming gcc release introduces a new warning for string operations reading beyond the end of a fixed-length object. After testing randconfig kernels for a while, think I have patches for any such warnings that came up on x86, arm and arm64. Most of these warnings are false-positive ones, either gcc warning about something that is entirely correct, or about something that looks suspicious but turns out to be correct after all. The two patches for the i915 driver look like something that might be actual bugs, but I am not sure about those either. We probably want some combination of workaround like the ones I post here and changes to gcc to have fewer false positives in the release. I'm posting the entire set of workaround that give me a cleanly building kernel for reference here. Arnd Arnd Bergmann (11): x86: compressed: avoid gcc-11 -Wstringop-overread warning x86: tboot: avoid Wstringop-overread-warning security: commoncap: fix -Wstringop-overread warning ath11: Wstringop-overread warning qnx: avoid -Wstringop-overread warning cgroup: fix -Wzero-length-bounds warnings ARM: sharpsl_param: work around -Wstringop-overread warning atmel: avoid gcc -Wstringop-overflow warning scsi: lpfc: fix gcc -Wstringop-overread warning drm/i915: avoid stringop-overread warning on pri_latency [RFC] drm/i915/dp: fix array overflow warning arch/arm/common/sharpsl_param.c | 4 ++- arch/x86/boot/compressed/misc.c | 2 -- arch/x86/kernel/tboot.c | 44 +++-- drivers/gpu/drm/i915/display/intel_dp.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 6 ++-- drivers/net/wireless/ath/ath11k/mac.c | 2 +- drivers/net/wireless/atmel/atmel.c | 25 -- drivers/scsi/lpfc/lpfc_attr.c | 6 ++-- fs/qnx4/dir.c | 11 +++ kernel/cgroup/cgroup.c | 15 +++-- security/commoncap.c| 2 +- 11 files changed, 69 insertions(+), 50 deletions(-) Cc: x...@kernel.org Cc: Ning Sun Cc: Jani Nikula Cc: Kalle Valo Cc: Simon Kelley Cc: James Smart Cc: "James E.J. Bottomley" Cc: Anders Larsen Cc: Tejun Heo Cc: Serge Hallyn Cc: Imre Deak Cc: linux-arm-ker...@lists.infradead.org Cc: linux-ker...@vger.kernel.org Cc: tboot-de...@lists.sourceforge.net Cc: intel-...@lists.freedesktop.org Cc: dri-de...@lists.freedesktop.org Cc: ath...@lists.infradead.org Cc: linux-wirel...@vger.kernel.org Cc: netdev@vger.kernel.org Cc: linux-s...@vger.kernel.org Cc: cgro...@vger.kernel.org Cc: linux-security-mod...@vger.kernel.org -- 2.29.2
Re: [PATCH net-next] [v2] misdn: avoid -Wempty-body warning
On Mon, Mar 22, 2021 at 2:58 PM Leon Romanovsky wrote: > > Thanks, interesting when we will delete whole drivers/isdn :) > > Reviewed-by: Leon Romanovsky Not any time soon I think, Harald Welte mentioned that Osmocom still relies on mISDN. The CAPI stuff only remains because of net/bluetooth/cmtp/ though. I don't think there are any users, but Marcel wanted to keep cmtp since it is not really hardware specific. We could probably move drivers/isdn/capi/* to net/bluetooth/cmtp/ if there was a good reason. Arnd
Re: [PATCH net-next 1/5] misdn: avoid -Wempty-body warning
On Mon, Mar 22, 2021 at 1:08 PM Leon Romanovsky wrote: > On Mon, Mar 22, 2021 at 12:24:20PM +0100, Arnd Bergmann wrote: > > On Mon, Mar 22, 2021 at 11:55 AM Leon Romanovsky wrote: > > > > I don't care either way, I only kept it because it was apparently left there > > on purpose by the original author, as seen by the comment. > > I personally would delete it. Ok, I sent a second version now, I hope one of them can get applied. Arnd
[PATCH net-next] [v2] misdn: avoid -Wempty-body warning
From: Arnd Bergmann
gcc warns about a pointless condition:
drivers/isdn/hardware/mISDN/hfcmulti.c: In function 'hfcmulti_interrupt':
drivers/isdn/hardware/mISDN/hfcmulti.c:2752:17: error: suggest braces around
empty body in an 'if' statement [-Werror=empty-body]
2752 | ; /* external IRQ */
As the check has no effect, just remove it.
Suggested-by: Leon Romanovsky
Signed-off-by: Arnd Bergmann
---
v2: remove the line instead of adding {}
---
drivers/isdn/hardware/mISDN/hfcmulti.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/isdn/hardware/mISDN/hfcmulti.c
b/drivers/isdn/hardware/mISDN/hfcmulti.c
index 7013a3f08429..14092152b786 100644
--- a/drivers/isdn/hardware/mISDN/hfcmulti.c
+++ b/drivers/isdn/hardware/mISDN/hfcmulti.c
@@ -2748,8 +2748,6 @@ hfcmulti_interrupt(int intno, void *dev_id)
if (hc->ctype != HFC_TYPE_E1)
ph_state_irq(hc, r_irq_statech);
}
- if (status & V_EXT_IRQSTA)
- ; /* external IRQ */
if (status & V_LOST_STA) {
/* LOST IRQ */
HFC_outb(hc, R_INC_RES_FIFO, V_RES_LOST); /* clear irq! */
--
2.29.2
[PATCH net-next] ipv6: fix clang Wformat warning
From: Arnd Bergmann
When building with 'make W=1', clang warns about a mismatched
format string:
net/ipv6/ah6.c:710:4: error: format specifies type 'unsigned short' but the
argument has type 'int' [-Werror,-Wformat]
aalg_desc->uinfo.auth.icv_fullbits/8);
^~~~
include/linux/printk.h:375:34: note: expanded from macro 'pr_info'
printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
~~~ ^~~
net/ipv6/esp6.c:1153:5: error: format specifies type 'unsigned short' but the
argument has type 'int' [-Werror,-Wformat]
aalg_desc->uinfo.auth.icv_fullbits / 8);
^~
include/linux/printk.h:375:34: note: expanded from macro 'pr_info'
printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
~~~ ^~~
Here, the result of dividing a 16-bit number by a 32-bit number
produces a 32-bit result, which is printed as a 16-bit integer.
Change the %hu format to the normal %u, which has the same effect
but avoids the warning.
Signed-off-by: Arnd Bergmann
---
net/ipv6/ah6.c | 2 +-
net/ipv6/esp6.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index 440080da805b..01c638f5d8b8 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -705,7 +705,7 @@ static int ah6_init_state(struct xfrm_state *x)
if (aalg_desc->uinfo.auth.icv_fullbits/8 !=
crypto_ahash_digestsize(ahash)) {
- pr_info("AH: %s digestsize %u != %hu\n",
+ pr_info("AH: %s digestsize %u != %u\n",
x->aalg->alg_name, crypto_ahash_digestsize(ahash),
aalg_desc->uinfo.auth.icv_fullbits/8);
goto error;
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index 153ad103ba74..831a588b04a2 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -1147,7 +1147,7 @@ static int esp_init_authenc(struct xfrm_state *x)
err = -EINVAL;
if (aalg_desc->uinfo.auth.icv_fullbits / 8 !=
crypto_aead_authsize(aead)) {
- pr_info("ESP: %s digestsize %u != %hu\n",
+ pr_info("ESP: %s digestsize %u != %u\n",
x->aalg->alg_name,
crypto_aead_authsize(aead),
aalg_desc->uinfo.auth.icv_fullbits / 8);
--
2.29.2
Re: [PATCH net-next 1/5] misdn: avoid -Wempty-body warning
On Mon, Mar 22, 2021 at 11:55 AM Leon Romanovsky wrote:
> On Mon, Mar 22, 2021 at 11:43:31AM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann
> >
> > gcc warns about a pointless condition:
> >
> > drivers/isdn/hardware/mISDN/hfcmulti.c: In function 'hfcmulti_interrupt':
> > drivers/isdn/hardware/mISDN/hfcmulti.c:2752:17: error: suggest braces
> > around empty body in an 'if' statement [-Werror=empty-body]
> > 2752 | ; /* external IRQ */
> >
> > Change this as suggested by gcc, which also fits the style of the
> > other conditions in this function.
> >
> > Signed-off-by: Arnd Bergmann
> > ---
> > drivers/isdn/hardware/mISDN/hfcmulti.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/isdn/hardware/mISDN/hfcmulti.c
> > b/drivers/isdn/hardware/mISDN/hfcmulti.c
> > index 7013a3f08429..8ab0fde758d2 100644
> > --- a/drivers/isdn/hardware/mISDN/hfcmulti.c
> > +++ b/drivers/isdn/hardware/mISDN/hfcmulti.c
> > @@ -2748,8 +2748,9 @@ hfcmulti_interrupt(int intno, void *dev_id)
> > if (hc->ctype != HFC_TYPE_E1)
> > ph_state_irq(hc, r_irq_statech);
> > }
> > - if (status & V_EXT_IRQSTA)
> > - ; /* external IRQ */
> > + if (status & V_EXT_IRQSTA) {
> > + /* external IRQ */
> > + }
>
> Any reason do not delete this hunk?
I don't care either way, I only kept it because it was apparently left there
on purpose by the original author, as seen by the comment.
Arnd
[PATCH net-next 5/5] vxge: avoid -Wemtpy-body warnings
From: Arnd Bergmann
There are a few warnings about empty debug macros in this driver:
drivers/net/ethernet/neterion/vxge/vxge-main.c: In function 'vxge_probe':
drivers/net/ethernet/neterion/vxge/vxge-main.c:4480:76: error: suggest braces
around empty body in an 'if' statement [-Werror=empty-body]
4480 | "Failed in enabling SRIOV mode: %d\n",
ret);
Change them to proper 'do { } while (0)' expressions to make the
code a little more robust and avoid the warnings.
Signed-off-by: Arnd Bergmann
---
drivers/net/ethernet/neterion/vxge/vxge-main.h | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/neterion/vxge/vxge-main.h
b/drivers/net/ethernet/neterion/vxge/vxge-main.h
index 9c86f4f9cd42..55673fdebe3e 100644
--- a/drivers/net/ethernet/neterion/vxge/vxge-main.h
+++ b/drivers/net/ethernet/neterion/vxge/vxge-main.h
@@ -454,49 +454,49 @@ int vxge_fw_upgrade(struct vxgedev *vdev, char *fw_name,
int override);
#define vxge_debug_ll_config(level, fmt, ...) \
vxge_debug_ll(level, VXGE_DEBUG_LL_CONFIG, fmt, ##__VA_ARGS__)
#else
-#define vxge_debug_ll_config(level, fmt, ...)
+#define vxge_debug_ll_config(level, fmt, ...) no_printk(fmt)
#endif
#if (VXGE_DEBUG_INIT & VXGE_DEBUG_MASK)
#define vxge_debug_init(level, fmt, ...) \
vxge_debug_ll(level, VXGE_DEBUG_INIT, fmt, ##__VA_ARGS__)
#else
-#define vxge_debug_init(level, fmt, ...)
+#define vxge_debug_init(level, fmt, ...) no_printk(fmt)
#endif
#if (VXGE_DEBUG_TX & VXGE_DEBUG_MASK)
#define vxge_debug_tx(level, fmt, ...) \
vxge_debug_ll(level, VXGE_DEBUG_TX, fmt, ##__VA_ARGS__)
#else
-#define vxge_debug_tx(level, fmt, ...)
+#define vxge_debug_tx(level, fmt, ...) no_printk(fmt)
#endif
#if (VXGE_DEBUG_RX & VXGE_DEBUG_MASK)
#define vxge_debug_rx(level, fmt, ...) \
vxge_debug_ll(level, VXGE_DEBUG_RX, fmt, ##__VA_ARGS__)
#else
-#define vxge_debug_rx(level, fmt, ...)
+#define vxge_debug_rx(level, fmt, ...) no_printk(fmt)
#endif
#if (VXGE_DEBUG_MEM & VXGE_DEBUG_MASK)
#define vxge_debug_mem(level, fmt, ...) \
vxge_debug_ll(level, VXGE_DEBUG_MEM, fmt, ##__VA_ARGS__)
#else
-#define vxge_debug_mem(level, fmt, ...)
+#define vxge_debug_mem(level, fmt, ...) no_printk(fmt)
#endif
#if (VXGE_DEBUG_ENTRYEXIT & VXGE_DEBUG_MASK)
#define vxge_debug_entryexit(level, fmt, ...) \
vxge_debug_ll(level, VXGE_DEBUG_ENTRYEXIT, fmt, ##__VA_ARGS__)
#else
-#define vxge_debug_entryexit(level, fmt, ...)
+#define vxge_debug_entryexit(level, fmt, ...) no_printk(fmt)
#endif
#if (VXGE_DEBUG_INTR & VXGE_DEBUG_MASK)
#define vxge_debug_intr(level, fmt, ...) \
vxge_debug_ll(level, VXGE_DEBUG_INTR, fmt, ##__VA_ARGS__)
#else
-#define vxge_debug_intr(level, fmt, ...)
+#define vxge_debug_intr(level, fmt, ...) no_printk(fmt)
#endif
#define VXGE_DEVICE_DEBUG_LEVEL_SET(level, mask, vdev) {\
--
2.29.2
[PATCH net-next 4/5] libertas: avoid -Wempty-body warning
From: Arnd Bergmann
Building without mesh supports shows a couple of warnings with
'make W=1':
drivers/net/wireless/marvell/libertas/main.c: In function 'lbs_start_card':
drivers/net/wireless/marvell/libertas/main.c:1068:37: error: suggest braces
around empty body in an 'if' statement [-Werror=empty-body]
1068 | lbs_start_mesh(priv);
Change the macros to use the usual "do { } while (0)" instead to shut up
the warnings and make the code a litte more robust.
Signed-off-by: Arnd Bergmann
---
drivers/net/wireless/marvell/libertas/mesh.h | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/marvell/libertas/mesh.h
b/drivers/net/wireless/marvell/libertas/mesh.h
index d49717b20c09..44c4cd0230a8 100644
--- a/drivers/net/wireless/marvell/libertas/mesh.h
+++ b/drivers/net/wireless/marvell/libertas/mesh.h
@@ -60,13 +60,13 @@ void lbs_mesh_ethtool_get_strings(struct net_device *dev,
#else
-#define lbs_init_mesh(priv)
-#define lbs_deinit_mesh(priv)
-#define lbs_start_mesh(priv)
-#define lbs_add_mesh(priv)
-#define lbs_remove_mesh(priv)
+#define lbs_init_mesh(priv)do { } while (0)
+#define lbs_deinit_mesh(priv) do { } while (0)
+#define lbs_start_mesh(priv) do { } while (0)
+#define lbs_add_mesh(priv) do { } while (0)
+#define lbs_remove_mesh(priv) do { } while (0)
#define lbs_mesh_set_dev(priv, dev, rxpd) (dev)
-#define lbs_mesh_set_txpd(priv, dev, txpd)
+#define lbs_mesh_set_txpd(priv, dev, txpd) do { } while (0)
#define lbs_mesh_set_channel(priv, channel) (0)
#define lbs_mesh_activated(priv) (false)
--
2.29.2
[PATCH net-next 3/5] iwlegacy: avoid -Wempty-body warning
From: Arnd Bergmann
There are a couple of warnings in this driver when building with W=1:
drivers/net/wireless/intel/iwlegacy/common.c: In function 'il_power_set_mode':
drivers/net/wireless/intel/iwlegacy/common.c:1195:60: error: suggest braces
around empty body in an 'if' statement [-Werror=empty-body]
1195 | il->chain_noise_data.state);
|^
drivers/net/wireless/intel/iwlegacy/common.c: In function 'il_do_scan_abort':
drivers/net/wireless/intel/iwlegacy/common.c:1343:57: error: suggest braces
around empty body in an 'else' statement [-Werror=empty-body]
Change the empty debug macros to no_printk(), which avoids the
warnings and adds useful format string checks.
Signed-off-by: Arnd Bergmann
---
drivers/net/wireless/intel/iwlegacy/3945-mac.c | 2 --
drivers/net/wireless/intel/iwlegacy/common.c | 2 --
drivers/net/wireless/intel/iwlegacy/common.h | 2 +-
3 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlegacy/3945-mac.c
b/drivers/net/wireless/intel/iwlegacy/3945-mac.c
index 4ca8212d4fa4..6ff2674f8466 100644
--- a/drivers/net/wireless/intel/iwlegacy/3945-mac.c
+++ b/drivers/net/wireless/intel/iwlegacy/3945-mac.c
@@ -751,9 +751,7 @@ il3945_hdl_alive(struct il_priv *il, struct il_rx_buf *rxb)
static void
il3945_hdl_add_sta(struct il_priv *il, struct il_rx_buf *rxb)
{
-#ifdef CONFIG_IWLEGACY_DEBUG
struct il_rx_pkt *pkt = rxb_addr(rxb);
-#endif
D_RX("Received C_ADD_STA: 0x%02X\n", pkt->u.status);
}
diff --git a/drivers/net/wireless/intel/iwlegacy/common.c
b/drivers/net/wireless/intel/iwlegacy/common.c
index 0651a6a416d1..219fed91cac5 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.c
+++ b/drivers/net/wireless/intel/iwlegacy/common.c
@@ -1430,10 +1430,8 @@ static void
il_hdl_scan_complete(struct il_priv *il, struct il_rx_buf *rxb)
{
-#ifdef CONFIG_IWLEGACY_DEBUG
struct il_rx_pkt *pkt = rxb_addr(rxb);
struct il_scancomplete_notification *scan_notif = (void *)pkt->u.raw;
-#endif
D_SCAN("Scan complete: %d channels (TSF 0x%08X:%08X) - %d\n",
scan_notif->scanned_channels, scan_notif->tsf_low,
diff --git a/drivers/net/wireless/intel/iwlegacy/common.h
b/drivers/net/wireless/intel/iwlegacy/common.h
index ea1b1bb7ddcb..40877ef1fbf2 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.h
+++ b/drivers/net/wireless/intel/iwlegacy/common.h
@@ -2937,7 +2937,7 @@ do {
\
} while (0)
#else
-#define IL_DBG(level, fmt, args...)
+#define IL_DBG(level, fmt, args...) no_printk(fmt, ##args)
static inline void
il_print_hex_dump(struct il_priv *il, int level, const void *p, u32 len)
{
--
2.29.2
[PATCH net-next 2/5] dccp: avoid Wempty-body warning
From: Arnd Bergmann
There are a couple of warnings in this driver when building with W=1:
net/dccp/output.c: In function 'dccp_xmit_packet':
net/dccp/output.c:283:71: error: suggest braces around empty body in an 'if'
statement [-Werror=empty-body]
283 | dccp_pr_debug("transmit_skb() returned err=%d\n", err);
| ^
net/dccp/ackvec.c: In function 'dccp_ackvec_update_old':
net/dccp/ackvec.c:163:80: error: suggest braces around empty body in an 'else'
statement [-Werror=empty-body]
163 | (unsigned long
long)seqno, state);
|
^
Change the empty debug macros to no_printk(), which avoids the
warnings and adds useful format string checks.
Signed-off-by: Arnd Bergmann
---
net/dccp/dccp.h | 6 +++---
net/dccp/proto.c | 2 --
2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/net/dccp/dccp.h b/net/dccp/dccp.h
index 9cc9d1ee6cdb..8a5163620bc3 100644
--- a/net/dccp/dccp.h
+++ b/net/dccp/dccp.h
@@ -41,9 +41,9 @@ extern bool dccp_debug;
#define dccp_pr_debug_cat(format, a...) DCCP_PRINTK(dccp_debug, format, ##a)
#define dccp_debug(fmt, a...)dccp_pr_debug_cat(KERN_DEBUG fmt, ##a)
#else
-#define dccp_pr_debug(format, a...)
-#define dccp_pr_debug_cat(format, a...)
-#define dccp_debug(format, a...)
+#define dccp_pr_debug(format, a...) no_printk(format, ##a)
+#define dccp_pr_debug_cat(format, a...) no_printk(format, ##a)
+#define dccp_debug(format, a...) no_printk(format, ##a)
#endif
extern struct inet_hashinfo dccp_hashinfo;
diff --git a/net/dccp/proto.c b/net/dccp/proto.c
index 6d705d90c614..97a175eaf247 100644
--- a/net/dccp/proto.c
+++ b/net/dccp/proto.c
@@ -51,7 +51,6 @@ EXPORT_SYMBOL_GPL(dccp_hashinfo);
/* the maximum queue length for tx in packets. 0 is no limit */
int sysctl_dccp_tx_qlen __read_mostly = 5;
-#ifdef CONFIG_IP_DCCP_DEBUG
static const char *dccp_state_name(const int state)
{
static const char *const dccp_state_names[] = {
@@ -73,7 +72,6 @@ static const char *dccp_state_name(const int state)
else
return dccp_state_names[state];
}
-#endif
void dccp_set_state(struct sock *sk, const int state)
{
--
2.29.2
[PATCH net-next 1/5] misdn: avoid -Wempty-body warning
From: Arnd Bergmann
gcc warns about a pointless condition:
drivers/isdn/hardware/mISDN/hfcmulti.c: In function 'hfcmulti_interrupt':
drivers/isdn/hardware/mISDN/hfcmulti.c:2752:17: error: suggest braces around
empty body in an 'if' statement [-Werror=empty-body]
2752 | ; /* external IRQ */
Change this as suggested by gcc, which also fits the style of the
other conditions in this function.
Signed-off-by: Arnd Bergmann
---
drivers/isdn/hardware/mISDN/hfcmulti.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/isdn/hardware/mISDN/hfcmulti.c
b/drivers/isdn/hardware/mISDN/hfcmulti.c
index 7013a3f08429..8ab0fde758d2 100644
--- a/drivers/isdn/hardware/mISDN/hfcmulti.c
+++ b/drivers/isdn/hardware/mISDN/hfcmulti.c
@@ -2748,8 +2748,9 @@ hfcmulti_interrupt(int intno, void *dev_id)
if (hc->ctype != HFC_TYPE_E1)
ph_state_irq(hc, r_irq_statech);
}
- if (status & V_EXT_IRQSTA)
- ; /* external IRQ */
+ if (status & V_EXT_IRQSTA) {
+ /* external IRQ */
+ }
if (status & V_LOST_STA) {
/* LOST IRQ */
HFC_outb(hc, R_INC_RES_FIFO, V_RES_LOST); /* clear irq! */
--
2.29.2
Re: GTE - The hardware timestamping engine
On Sat, Mar 20, 2021 at 12:56 PM Linus Walleij wrote:
>
> Hi Dipen,
>
> thanks for your mail!
>
> I involved some other kernel people to get some discussion.
> I think Kent Gibson can be of great help because he is using
> GPIOs with high precision.
>
> We actually discussed this a bit when adding support for
> realtime timestamps.
Adding Richard Cochran as well, for drivers/ptp/, he may be able to
identify whether this should be integrated into that framework in some
form.
fullquote below
> On Wed, Mar 17, 2021 at 11:29 PM Dipen Patel wrote:
>
> > Nvidia Tegra SoCs have generic timestamping engine (GTE) hardware module
> > which
> > can monitor SoC signals like IRQ lines and GPIO lines for state change, upon
> > detecting the change, it can timestamp and store in its internal hardware
> > FIFO.
> > The advantage of the GTE module can be realized in applications like
> > robotics
> > or autonomous vehicle where it can help record events with precise
> > timestamp.
>
> That sounds very useful.
>
> Certainly the kernel shall be able to handle this.
>
> >
> > For GPIO:
> >
> > 1. GPIO has to be configured as input and IRQ must be enabled.
> > 2. Ask GPIO controller driver to set corresponding timestamp bit in the
> > specified GPIO config register.
> > 3. Translate GPIO specified by the client to its internal bitmap.
> > 3.a For example, If client specifies GPIO line 31, it could be bit 13 of GTE
> > register.
> > 4. Set internal bits to enable monitoring in GTE module
> > 5. Additionally GTE driver can open up lanes for the user space application
> > as a client and can send timestamping events directly to the
> > application.
>
> I have some concerns:
>
> 1. GPIO should for all professional applications be used with the character
> device /dev/gpiochipN, under no circumstances shall the old sysfs
> ABI be used for this. In this case it is necessary because the
> character device provides events in a FIFO to userspace, which is
> what we need.
>
> The timestamp provided to userspace is an opaque 64bit
> unsigned value. I suppose we assume it is monotonic but
> you can actually augment the semantics for your specific
> stamp, as long as 64 bits is gonna work.
>
> 2. The timestamp for the chardev is currently obtained in
> drivers/gpio/gpiolib-cdev.c like this:
>
> static u64 line_event_timestamp(struct line *line)
> {
> if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &line->desc->flags))
> return ktime_get_real_ns();
>
> return ktime_get_ns();
> }
>
> What you want to do is to add a new flag for hardware timestamps
> and use that if available. FLAG_EVENT_CLOCK_HARDWARE?
> FLAG_EVENT_CLOCK_NATIVE?
>
> Then you need to figure out a mechanism so we can obtain
> the right timestamp from the hardware event right here,
> you can hook into the GPIO driver if need be, we can
> figure out the gpio_chip for a certain line for sure.
>
> So you first need to augment the userspace
> ABI and the character device code to add this. See
> commit 26d060e47e25f2c715a1b2c48fea391f67907a30
> "gpiolib: cdev: allow edge event timestamps to be configured as REALTIME"
> by Kent Gibson to see what needs to be done.
>
> 3. Also patch tools/gpio/gpio-event-mon.c to support this flag and use that
> for prototyping and proof of concept.
>
> >
> > For IRQ:
> >
>
> Marc Zyngier and/or Thomas Gleixner know this stuff.
>
> It does make sense to add some infrastructure so that GPIO events
> and IRQs can use the same timestamping hardware.
>
> And certainly you will also want to use this timestamp for
> IIO devices? If it is just GPIOs and IRQs today, it will be
> gyroscopes and accelerometers tomorrow, am I right?
>
> Yours,
> Linus Walleij
Re: [RFC v2 3/5] arm64: socfpga: rename ARCH_STRATIX10 to ARCH_SOCFPGA64
On Thu, Mar 11, 2021 at 8:08 AM Krzysztof Kozlowski wrote: > On 10/03/2021 17:42, Arnd Bergmann wrote: > > On Wed, Mar 10, 2021 at 4:54 PM Krzysztof Kozlowski > > wrote: > >> On 10/03/2021 16:47, Krzysztof Kozlowski wrote: > >>> This edac Altera driver is very weird... it uses the same compatible > >>> differently depending whether this is 32-bit or 64-bit (e.g. Stratix > >>> 10)! On ARMv7 the compatible means for example one IRQ... On ARMv8, we > >>> have two. It's quite a new code (2019 from Intel), not some ancient > >>> legacy, so it should never have been accepted... > >> > >> Oh, it's not that horrible as it sounds. They actually have different > >> compatibles for edac driver with these differences (e.g. in interrupts). > >> They just do not use them and instead check for the basic (common?) > >> compatible and architecture... Anyway without testing I am not the > >> person to fix the edac driver. > > > > Ok, This should be fixed properly as you describe, but as a quick hack > > it wouldn't be hard to just change the #ifdef to check for CONFIG_64BIT > > instead of CONFIG_ARCH_STRATIX10 during the rename of the config > > symbol. > > This would work. The trouble with renaming ARCH_SOCFPGA into > ARCH_INTEL_SOCFPGA is that still SOCFPGA will appear in many other > Kconfig symbols or even directory paths. > > Let me use ARCH_INTEL_SOCFPGA for 64bit here and renaming of 32bit a > little bit later. Maybe you can introduce a hidden 'ARCH_INTEL_SOCFPGA' option first and select that from both the 32-bit and the 64-bit platforms in the first step. That should decouple the cleanups, so you can change the drivers to (only) 'depends on ARCH_INTEL_SOCFPGA' before removing the other names. Arnd
Re: [RFC v2 5/5] clk: socfpga: allow compile testing of Stratix 10 / Agilex clocks
On Wed, Mar 10, 2021 at 9:38 AM Krzysztof Kozlowski wrote: > --- a/drivers/clk/socfpga/Kconfig > +++ b/drivers/clk/socfpga/Kconfig > @@ -1,6 +1,17 @@ > # SPDX-License-Identifier: GPL-2.0 > +config COMMON_CLK_SOCFPGA > + bool "Intel SoCFPGA family clock support" if COMPILE_TEST && > !ARCH_SOCFPGA && !ARCH_SOCFPGA64 > + depends on ARCH_SOCFPGA || ARCH_SOCFPGA64 || COMPILE_TEST > + default y if ARCH_SOCFPGA || ARCH_SOCFPGA64 I think the 'depends on' line here is redundant if you also have the 'if' line and the default. Arnd
Re: [RFC v2 3/5] arm64: socfpga: rename ARCH_STRATIX10 to ARCH_SOCFPGA64
On Wed, Mar 10, 2021 at 4:54 PM Krzysztof Kozlowski wrote: > On 10/03/2021 16:47, Krzysztof Kozlowski wrote: > > This edac Altera driver is very weird... it uses the same compatible > > differently depending whether this is 32-bit or 64-bit (e.g. Stratix > > 10)! On ARMv7 the compatible means for example one IRQ... On ARMv8, we > > have two. It's quite a new code (2019 from Intel), not some ancient > > legacy, so it should never have been accepted... > > Oh, it's not that horrible as it sounds. They actually have different > compatibles for edac driver with these differences (e.g. in interrupts). > They just do not use them and instead check for the basic (common?) > compatible and architecture... Anyway without testing I am not the > person to fix the edac driver. Ok, This should be fixed properly as you describe, but as a quick hack it wouldn't be hard to just change the #ifdef to check for CONFIG_64BIT instead of CONFIG_ARCH_STRATIX10 during the rename of the config symbol. Arnd
Re: [RFC v2 3/5] arm64: socfpga: rename ARCH_STRATIX10 to ARCH_SOCFPGA64
On Wed, Mar 10, 2021 at 4:06 PM Krzysztof Kozlowski wrote: > On 10/03/2021 15:45, Tom Rix wrote: > > On 3/10/21 1:45 AM, Lee Jones wrote: > > Many other architectures do not have vendor prefix (TEGRA, EXYNOS, > ZYNQMP etc). I would call it the same as in ARMv7 - ARCH_SOCFPGA - but > the Altera EDAC driver depends on these symbols to be different. > Anyway, I don't mind using something else for the name. I agree the name SOCFPGA is confusing, since it is really a class of device that is made by multiple manufacturers rather than a brand name, but renaming that now would be equally confusing. If the Intel folks could suggest a better name that describes all products in the platform and is less ambiguous, we could rename it to that. I think ARCH_ALTERA would make sense, but I don't know if that is a name that is getting phased out. (We once renamed the Marvell Orion platform to ARCH_MVEBU, but shortly afterwards, Marvell renamed their embedded business unit (EBU) and the name has no longer made sense since). Regardless of what name we end up with, I do think we should have the same name for 32-bit and 64-bit and instead fix the edac driver to do runtime detection based on the compatible string. Arnd
[PATCH] net/mlx5e: allocate 'indirection_rqt' buffer dynamically
From: Arnd Bergmann
Increasing the size of the indirection_rqt array from 128 to 256 bytes
pushed the stack usage of the mlx5e_hairpin_fill_rqt_rqns() function
over the warning limit when building with clang and CONFIG_KASAN:
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:970:1: error: stack frame size
of 1180 bytes in function 'mlx5e_tc_add_nic_flow' [-Werror,-Wframe-larger-than=]
Using dynamic allocation here is safe because the caller does the
same, and it reduces the stack usage of the function to just a few
bytes.
Fixes: 1dd55ba2fb70 ("net/mlx5e: Increase indirection RQ table size to 256")
Signed-off-by: Arnd Bergmann
---
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 0da69b98f38f..66f98618dc13 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -445,12 +445,16 @@ static void mlx5e_hairpin_destroy_transport(struct
mlx5e_hairpin *hp)
mlx5_core_dealloc_transport_domain(hp->func_mdev, hp->tdn);
}
-static void mlx5e_hairpin_fill_rqt_rqns(struct mlx5e_hairpin *hp, void *rqtc)
+static int mlx5e_hairpin_fill_rqt_rqns(struct mlx5e_hairpin *hp, void *rqtc)
{
- u32 indirection_rqt[MLX5E_INDIR_RQT_SIZE], rqn;
+ u32 *indirection_rqt, rqn;
struct mlx5e_priv *priv = hp->func_priv;
int i, ix, sz = MLX5E_INDIR_RQT_SIZE;
+ indirection_rqt = kzalloc(sz, GFP_KERNEL);
+ if (!indirection_rqt)
+ return -ENOMEM;
+
mlx5e_build_default_indir_rqt(indirection_rqt, sz,
hp->num_channels);
@@ -462,6 +466,9 @@ static void mlx5e_hairpin_fill_rqt_rqns(struct
mlx5e_hairpin *hp, void *rqtc)
rqn = hp->pair->rqn[ix];
MLX5_SET(rqtc, rqtc, rq_num[i], rqn);
}
+
+ kfree(indirection_rqt);
+ return 0;
}
static int mlx5e_hairpin_create_indirect_rqt(struct mlx5e_hairpin *hp)
@@ -482,12 +489,15 @@ static int mlx5e_hairpin_create_indirect_rqt(struct
mlx5e_hairpin *hp)
MLX5_SET(rqtc, rqtc, rqt_actual_size, sz);
MLX5_SET(rqtc, rqtc, rqt_max_size, sz);
- mlx5e_hairpin_fill_rqt_rqns(hp, rqtc);
+ err = mlx5e_hairpin_fill_rqt_rqns(hp, rqtc);
+ if (err)
+ goto out;
err = mlx5_core_create_rqt(mdev, in, inlen, &hp->indir_rqt.rqtn);
if (!err)
hp->indir_rqt.enabled = true;
+out:
kvfree(in);
return err;
}
--
2.29.2
[PATCH] net/mlx5e: include net/nexthop.h where needed
From: Arnd Bergmann
drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c:1510:12: error:
implicit declaration of function 'fib_info_nh'
[-Werror,-Wimplicit-function-declaration]
fib_dev = fib_info_nh(fen_info->fi, 0)->fib_nh_dev;
^
drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c:1510:12: note: did
you mean 'fib_info_put'?
include/net/ip_fib.h:529:20: note: 'fib_info_put' declared here
static inline void fib_info_put(struct fib_info *fi)
^
Fixes: 8914add2c9e5 ("net/mlx5e: Handle FIB events to update tunnel endpoint
device")
Signed-off-by: Arnd Bergmann
---
drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
index 6a116335bb21..32d06fe94acc 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
@@ -2,6 +2,7 @@
/* Copyright (c) 2021 Mellanox Technologies. */
#include
+#include
#include "tc_tun_encap.h"
#include "en_tc.h"
#include "tc_tun.h"
--
2.29.2
Re: [net 01/15] net/mlx5e: E-switch, Fix rate calculation for overflow
On Tue, Mar 2, 2021 at 1:52 AM Saeed Mahameed wrote:
> On Sat, 2021-02-27 at 13:14 +0100, Arnd Bergmann wrote:
> > On Fri, Feb 12, 2021 at 3:59 AM Saeed Mahameed
> > wrote:
> > >
> > > From: Parav Pandit
> > >
> > > rate_bytes_ps is a 64-bit field. It passed as 32-bit field to
> > > apply_police_params(). Due to this when police rate is higher
> > > than 4Gbps, 32-bit calculation ignores the carry. This results
> > > in incorrect rate configurationn the device.
> > >
> > > Fix it by performing 64-bit calculation.
> >
> > I just stumbled over this commit while looking at an unrelated
> > problem.
> >
> > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > > b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > > index dd0bfbacad47..717fbaa6ce73 100644
> > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > > @@ -5040,7 +5040,7 @@ static int apply_police_params(struct
> > > mlx5e_priv *priv, u64 rate,
> > > */
> > > if (rate) {
> > > rate = (rate * BITS_PER_BYTE) + 50;
> > > - rate_mbps = max_t(u32, do_div(rate, 100), 1);
> > > + rate_mbps = max_t(u64, do_div(rate, 100), 1);
> >
> > I think there are still multiple issues with this line:
> >
> > - Before commit 1fe3e3166b35 ("net/mlx5e: E-switch, Fix rate
> > calculation for
> > overflow"), it was trying to calculate rate divided by 100, but
> > now
> > it uses the remainder of the division rather than the quotient. I
> > assume
> > this was meant to use div_u64() instead of do_div().
> >
>
> Yes, I already have a patch lined up to fix this issue.
ok
> > - Both div_u64() and do_div() return a 32-bit number, and '1' is a
> > constant
> > that also comfortably fits into a 32-bit number, so changing the
> > max_t
> > to return a 64-bit type has no effect on the result
> >
>
> as of the above comment, we shouldn't be using the return value of
> do_div().
Ok, I was confused here because do_div() returns a 32-bit type,
and is called by div_u64(). Of course that was nonsense because
do_div() returns the 32-bit remainder, while the division result
remains 64-bit.
> > - The maximum of an arbitrary unsigned integer and '1' is either one
> > or zero,
> >so there doesn't need to be an expensive division here at all.
> > From the
> >comment it sounds like the intention was to use 'min_t()' instead
> > of 'max_t()'.
> >It has however used 'max_t' since the code was first introduced.
> >
>
> if the input rate is less that 1mbps then the quotient will be 0,
> otherwise we want the quotient, and we don't allow 0, so max_t(rate, 1)
> should be used, what am I missing ?
And I have no idea what I was thinking here, of course you are right
and there is no other bug.
Arnd
Re: [net 01/15] net/mlx5e: E-switch, Fix rate calculation for overflow
On Fri, Feb 12, 2021 at 3:59 AM Saeed Mahameed wrote:
>
> From: Parav Pandit
>
> rate_bytes_ps is a 64-bit field. It passed as 32-bit field to
> apply_police_params(). Due to this when police rate is higher
> than 4Gbps, 32-bit calculation ignores the carry. This results
> in incorrect rate configurationn the device.
>
> Fix it by performing 64-bit calculation.
I just stumbled over this commit while looking at an unrelated
problem.
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index dd0bfbacad47..717fbaa6ce73 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -5040,7 +5040,7 @@ static int apply_police_params(struct mlx5e_priv *priv,
> u64 rate,
> */
> if (rate) {
> rate = (rate * BITS_PER_BYTE) + 50;
> - rate_mbps = max_t(u32, do_div(rate, 100), 1);
> + rate_mbps = max_t(u64, do_div(rate, 100), 1);
I think there are still multiple issues with this line:
- Before commit 1fe3e3166b35 ("net/mlx5e: E-switch, Fix rate calculation for
overflow"), it was trying to calculate rate divided by 100, but now
it uses the remainder of the division rather than the quotient. I assume
this was meant to use div_u64() instead of do_div().
- Both div_u64() and do_div() return a 32-bit number, and '1' is a constant
that also comfortably fits into a 32-bit number, so changing the max_t
to return a 64-bit type has no effect on the result
- The maximum of an arbitrary unsigned integer and '1' is either one or zero,
so there doesn't need to be an expensive division here at all. From the
comment it sounds like the intention was to use 'min_t()' instead
of 'max_t()'.
It has however used 'max_t' since the code was first introduced.
Arnd
Re: [PATCH 3/3] net: dsa: mt7530: add GPIOLIB dependency
On Thu, Feb 25, 2021 at 4:52 PM DENG Qingfang wrote: > > Hi Arnd, > > On Thu, Feb 25, 2021 at 10:40 PM Arnd Bergmann wrote: > > > > From: Arnd Bergmann > > > > The new gpio support may be optional at runtime, but it requires > > building against gpiolib: > > > > ERROR: modpost: "gpiochip_get_data" [drivers/net/dsa/mt7530.ko] undefined! > > ERROR: modpost: "devm_gpiochip_add_data_with_key" > > [drivers/net/dsa/mt7530.ko] undefined! > > > > Add a Kconfig dependency to enforce this. > > I think wrapping the GPIO code block with #ifdef CONFIG_GPIOLIB ... > #endif may be a better idea. In practice there is little difference, as most configurations have GPIOLIB enabled anyway, in particular every configuration in which this driver is used. If you want to send an alternative patch to add the #ifdef, please include Reported-by: Arnd Bergmann Arnd
Re: [PATCH 2/3] net: dsa: tag_ocelot_8021q: fix driver dependency
On Thu, Feb 25, 2021 at 4:07 PM Vladimir Oltean wrote: > On Thu, Feb 25, 2021 at 03:49:08PM +0100, Arnd Bergmann wrote: > > On Thu, Feb 25, 2021 at 3:47 PM Arnd Bergmann wrote: > > > On Thu, Feb 25, 2021 at 3:43 PM Vladimir Oltean wrote: > > > > On Thu, Feb 25, 2021 at 03:38:32PM +0100, Arnd Bergmann wrote: > > > > > From: Arnd Bergmann > > > > > > > > > > When the ocelot driver code is in a library, the dsa tag > > > > I see the problem now, I should have written 'loadable module', not > > 'library'. > > Let me know if I should resend with a fixed changelog text. > > Ah, ok, things clicked into place now that you said 'module'. > So basically, your patch is the standard Kconfig incantation for 'if the > ocelot switch lib is built as module, build the tagger as module too', > plus some extra handling to allow NET_DSA_TAG_OCELOT_8021Q to still be y > or m when COMPILE_TEST is enabled, but it will be compiled in a > reduced-functionality mode, without MSCC_OCELOT_SWITCH_LIB, therefore > without PTP. > > Do I get things right? Sorry, Kconfig is a very strange language. Yes, that's basically correct. I tried to express it in Kconfig the way I would explain it in English, which means it there are two options: a) If MSCC_OCELOT_SWITCH_LIB is enabled (y or m) there is a direct dependency, so NET_DSA_TAG_OCELOT_8021Q cannot be built-in if MSCC_OCELOT_SWITCH_LIB=m b) When compile-testing *and* MSCC_OCELOT_SWITCH_LIB is fully disabled, NET_DSA_TAG_OCELOT_8021Q can be anything (y/m/n) As a side-effect, this also means that if we are not compile-testing and MSCC_OCELOT_SWITCH_LIB is disabled, the option is hdden. Arnd.
[PATCH 2/2] mt76: mt7921: remove incorrect error handling
From: Arnd Bergmann
Clang points out a mistake in the error handling in
mt7921_mcu_tx_rate_report(), which tries to dereference a pointer that
cannot be initialized because of the error that is being handled:
drivers/net/wireless/mediatek/mt76/mt7921/mcu.c:409:3: warning: variable
'stats' is uninitialized when used here [-Wuninitialized]
stats->tx_rate = rate;
^
drivers/net/wireless/mediatek/mt76/mt7921/mcu.c:401:32: note: initialize the
variable 'stats' to silence this warning
struct mt7921_sta_stats *stats;
^
Just remove the obviously incorrect line.
Fixes: 1c099ab44727 ("mt76: mt7921: add MCU support")
Signed-off-by: Arnd Bergmann
---
drivers/net/wireless/mediatek/mt76/mt7921/mcu.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/mcu.c
b/drivers/net/wireless/mediatek/mt76/mt7921/mcu.c
index db125cd22b91..b5cc72e7e81c 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/mcu.c
@@ -405,10 +405,8 @@ mt7921_mcu_tx_rate_report(struct mt7921_dev *dev, struct
sk_buff *skb,
if (wlan_idx >= MT76_N_WCIDS)
return;
wcid = rcu_dereference(dev->mt76.wcid[wlan_idx]);
- if (!wcid) {
- stats->tx_rate = rate;
+ if (!wcid)
return;
- }
msta = container_of(wcid, struct mt7921_sta, wcid);
stats = &msta->stats;
--
2.29.2
[PATCH 1/2] mt76: mt7915: fix unused 'mode' variable
From: Arnd Bergmann
clang points out a possible corner case in the mt7915_tm_set_tx_cont()
function if called with invalid arguments:
drivers/net/wireless/mediatek/mt76/mt7915/testmode.c:593:2: warning: variable
'mode' is used uninitialized whenever switch default is taken
[-Wsometimes-uninitialized]
default:
^~~
drivers/net/wireless/mediatek/mt76/mt7915/testmode.c:597:13: note:
uninitialized use occurs here
rateval = mode << 6 | rate_idx;
^~~~
drivers/net/wireless/mediatek/mt76/mt7915/testmode.c:506:37: note: initialize
the variable 'mode' to silence this warning
u8 rate_idx = td->tx_rate_idx, mode;
^
Change it to return an error instead of continuing with invalid data
here.
Fixes: 3f0caa3cbf94 ("mt76: mt7915: add support for continuous tx in testmode")
Signed-off-by: Arnd Bergmann
---
drivers/net/wireless/mediatek/mt76/mt7915/testmode.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7915/testmode.c
b/drivers/net/wireless/mediatek/mt76/mt7915/testmode.c
index 7fb2170a9561..aa629e1fb420 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7915/testmode.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7915/testmode.c
@@ -543,6 +543,7 @@ mt7915_tm_set_tx_cont(struct mt7915_phy *phy, bool en)
tx_cont->bw = CMD_CBW_20MHZ;
break;
default:
+ return -EINVAL;
break;
}
@@ -591,6 +592,7 @@ mt7915_tm_set_tx_cont(struct mt7915_phy *phy, bool en)
mode = MT_PHY_TYPE_HE_MU;
break;
default:
+ return -EINVAL;
break;
}
--
2.29.2
[PATCH] net: phy: make mdio_bus_phy_suspend/resume as __maybe_unused
From: Arnd Bergmann
When CONFIG_PM_SLEEP is disabled, the compiler warns about unused
functions:
drivers/net/phy/phy_device.c:273:12: error: unused function
'mdio_bus_phy_suspend' [-Werror,-Wunused-function]
static int mdio_bus_phy_suspend(struct device *dev)
drivers/net/phy/phy_device.c:293:12: error: unused function
'mdio_bus_phy_resume' [-Werror,-Wunused-function]
static int mdio_bus_phy_resume(struct device *dev)
The logic is intentional, so just mark these two as __maybe_unused
and remove the incorrect #ifdef.
Fixes: 4c0d2e96ba05 ("net: phy: consider that suspend2ram may cut off PHY
power")
Signed-off-by: Arnd Bergmann
---
drivers/net/phy/phy_device.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ce495473cd5d..cc38e326405a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -230,7 +230,6 @@ static struct phy_driver genphy_driver;
static LIST_HEAD(phy_fixup_list);
static DEFINE_MUTEX(phy_fixup_lock);
-#ifdef CONFIG_PM
static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
{
struct device_driver *drv = phydev->mdio.dev.driver;
@@ -270,7 +269,7 @@ static bool mdio_bus_phy_may_suspend(struct phy_device
*phydev)
return !phydev->suspended;
}
-static int mdio_bus_phy_suspend(struct device *dev)
+static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
{
struct phy_device *phydev = to_phy_device(dev);
@@ -290,7 +289,7 @@ static int mdio_bus_phy_suspend(struct device *dev)
return phy_suspend(phydev);
}
-static int mdio_bus_phy_resume(struct device *dev)
+static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
{
struct phy_device *phydev = to_phy_device(dev);
int ret;
@@ -316,7 +315,6 @@ static int mdio_bus_phy_resume(struct device *dev)
static SIMPLE_DEV_PM_OPS(mdio_bus_phy_pm_ops, mdio_bus_phy_suspend,
mdio_bus_phy_resume);
-#endif /* CONFIG_PM */
/**
* phy_register_fixup - creates a new phy_fixup and adds it to the list
--
2.29.2
Re: [PATCH 2/3] net: dsa: tag_ocelot_8021q: fix driver dependency
On Thu, Feb 25, 2021 at 3:47 PM Arnd Bergmann wrote: > On Thu, Feb 25, 2021 at 3:43 PM Vladimir Oltean wrote: > > On Thu, Feb 25, 2021 at 03:38:32PM +0100, Arnd Bergmann wrote: > > > From: Arnd Bergmann > > > > > > When the ocelot driver code is in a library, the dsa tag I see the problem now, I should have written 'loadable module', not 'library'. Let me know if I should resend with a fixed changelog text. Arnd
Re: [PATCH 2/3] net: dsa: tag_ocelot_8021q: fix driver dependency
On Thu, Feb 25, 2021 at 3:43 PM Vladimir Oltean wrote:
>
> On Thu, Feb 25, 2021 at 03:38:32PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann
> >
> > When the ocelot driver code is in a library, the dsa tag
> > code cannot be built-in:
> >
> > ld.lld: error: undefined symbol: ocelot_can_inject
> > >>> referenced by tag_ocelot_8021q.c
> > >>> dsa/tag_ocelot_8021q.o:(ocelot_xmit) in archive
> > >>> net/built-in.a
> >
> > ld.lld: error: undefined symbol: ocelot_port_inject_frame
> > >>> referenced by tag_ocelot_8021q.c
> > >>> dsa/tag_ocelot_8021q.o:(ocelot_xmit) in archive
> > >>> net/built-in.a
> >
> > Building the tag support only really makes sense for compile-testing
> > when the driver is available, so add a Kconfig dependency that prevents
> > the broken configuration while allowing COMPILE_TEST alternative when
> > MSCC_OCELOT_SWITCH_LIB is disabled entirely. This case is handled
> > through the #ifdef check in include/soc/mscc/ocelot.h.
> >
> > Fixes: 0a6f17c6ae21 ("net: dsa: tag_ocelot_8021q: add support for PTP
> > timestamping")
> > Signed-off-by: Arnd Bergmann
> > ---
> > net/dsa/Kconfig | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> > index 3589224c8da9..58b8fc82cd3c 100644
> > --- a/net/dsa/Kconfig
> > +++ b/net/dsa/Kconfig
> > @@ -118,6 +118,8 @@ config NET_DSA_TAG_OCELOT
> >
> > config NET_DSA_TAG_OCELOT_8021Q
> > tristate "Tag driver for Ocelot family of switches, using VLAN"
> > + depends on MSCC_OCELOT_SWITCH_LIB || \
> > + (MSCC_OCELOT_SWITCH_LIB=n && COMPILE_TEST)
> > select NET_DSA_TAG_8021Q
> > help
> > Say Y or M if you want to enable support for tagging frames with a
> > --
> > 2.29.2
> >
>
> Why isn't this code in include/soc/mscc/ocelot.h enough?
>
> #if IS_ENABLED(CONFIG_MSCC_OCELOT_SWITCH_LIB)
>
> bool ocelot_can_inject(struct ocelot *ocelot, int grp);
> void ocelot_port_inject_frame(struct ocelot *ocelot, int port, int grp,
> u32 rew_op, struct sk_buff *skb);
> int ocelot_xtr_poll_frame(struct ocelot *ocelot, int grp, struct sk_buff
> **skb);
> void ocelot_drain_cpu_queue(struct ocelot *ocelot, int grp);
>
> #else
>
> static inline bool ocelot_can_inject(struct ocelot *ocelot, int grp)
> {
> return false;
> }
That code is in include/soc/mscc/ocelot.h, it is what causes the
problem with CONFIG_MSCC_OCELOT_SWITCH_LIB=m
and NET_DSA_TAG_OCELOT_8021Q=y, as I tried to explain.
Arnd
[PATCH 3/3] net: dsa: mt7530: add GPIOLIB dependency
From: Arnd Bergmann
The new gpio support may be optional at runtime, but it requires
building against gpiolib:
ERROR: modpost: "gpiochip_get_data" [drivers/net/dsa/mt7530.ko] undefined!
ERROR: modpost: "devm_gpiochip_add_data_with_key" [drivers/net/dsa/mt7530.ko]
undefined!
Add a Kconfig dependency to enforce this.
Fixes: 429a0edeefd8 ("net: dsa: mt7530: MT7530 optional GPIO support")
Signed-off-by: Arnd Bergmann
---
drivers/net/dsa/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index 3af373e90806..07fc2a732597 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -37,6 +37,7 @@ config NET_DSA_LANTIQ_GSWIP
config NET_DSA_MT7530
tristate "MediaTek MT753x and MT7621 Ethernet switch support"
depends on NET_DSA
+ depends on GPIOLIB
select NET_DSA_TAG_MTK
help
This enables support for the MediaTek MT7530, MT7531, and MT7621
--
2.29.2
[PATCH 1/3] net: mscc: ocelot: select NET_DEVLINK
From: Arnd Bergmann
Without this option, the driver fails to link:
ld.lld: error: undefined symbol: devlink_sb_register
>>> referenced by ocelot_devlink.c
>>>
>>> net/ethernet/mscc/ocelot_devlink.o:(ocelot_devlink_sb_register) in archive
>>> drivers/built-in.a
>>> referenced by ocelot_devlink.c
>>>
>>> net/ethernet/mscc/ocelot_devlink.o:(ocelot_devlink_sb_register) in archive
>>> drivers/built-in.a
Fixes: f59fd9cab730 ("net: mscc: ocelot: configure watermarks using devlink-sb")
Signed-off-by: Arnd Bergmann
---
drivers/net/ethernet/mscc/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/mscc/Kconfig
b/drivers/net/ethernet/mscc/Kconfig
index c0ede0ca7115..05cb040c2677 100644
--- a/drivers/net/ethernet/mscc/Kconfig
+++ b/drivers/net/ethernet/mscc/Kconfig
@@ -13,6 +13,7 @@ if NET_VENDOR_MICROSEMI
# Users should depend on NET_SWITCHDEV, HAS_IOMEM
config MSCC_OCELOT_SWITCH_LIB
+ select NET_DEVLINK
select REGMAP_MMIO
select PACKING
select PHYLIB
--
2.29.2
[PATCH 2/3] net: dsa: tag_ocelot_8021q: fix driver dependency
From: Arnd Bergmann
When the ocelot driver code is in a library, the dsa tag
code cannot be built-in:
ld.lld: error: undefined symbol: ocelot_can_inject
>>> referenced by tag_ocelot_8021q.c
>>> dsa/tag_ocelot_8021q.o:(ocelot_xmit) in archive net/built-in.a
ld.lld: error: undefined symbol: ocelot_port_inject_frame
>>> referenced by tag_ocelot_8021q.c
>>> dsa/tag_ocelot_8021q.o:(ocelot_xmit) in archive net/built-in.a
Building the tag support only really makes sense for compile-testing
when the driver is available, so add a Kconfig dependency that prevents
the broken configuration while allowing COMPILE_TEST alternative when
MSCC_OCELOT_SWITCH_LIB is disabled entirely. This case is handled
through the #ifdef check in include/soc/mscc/ocelot.h.
Fixes: 0a6f17c6ae21 ("net: dsa: tag_ocelot_8021q: add support for PTP
timestamping")
Signed-off-by: Arnd Bergmann
---
net/dsa/Kconfig | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index 3589224c8da9..58b8fc82cd3c 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -118,6 +118,8 @@ config NET_DSA_TAG_OCELOT
config NET_DSA_TAG_OCELOT_8021Q
tristate "Tag driver for Ocelot family of switches, using VLAN"
+ depends on MSCC_OCELOT_SWITCH_LIB || \
+ (MSCC_OCELOT_SWITCH_LIB=n && COMPILE_TEST)
select NET_DSA_TAG_8021Q
help
Say Y or M if you want to enable support for tagging frames with a
--
2.29.2
[PATCH] iwlwifi: fix link error without CONFIG_IWLMVM
From: Arnd Bergmann
A runtime check that was introduced recently failed to
check for the matching Kconfig symbol:
ld.lld: error: undefined symbol: iwl_so_trans_cfg
>>> referenced by drv.c
>>> net/wireless/intel/iwlwifi/pcie/drv.o:(iwl_pci_probe)
Fixes: 930be4e76f26 ("iwlwifi: add support for SnJ with Jf devices")
Signed-off-by: Arnd Bergmann
---
drivers/net/wireless/intel/iwlwifi/pcie/drv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
index 314fec4a89ad..a2f5c4fc2324 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
@@ -1113,7 +1113,8 @@ static int iwl_pci_probe(struct pci_dev *pdev, const
struct pci_device_id *ent)
* here. But if we detect that the MAC type is actually SnJ,
* we should switch to it here to avoid problems later.
*/
- if (CSR_HW_REV_TYPE(iwl_trans->hw_rev) == IWL_CFG_MAC_TYPE_SNJ)
+ if (IS_ENABLED(CONFIG_IWLMVM) &&
+ CSR_HW_REV_TYPE(iwl_trans->hw_rev) == IWL_CFG_MAC_TYPE_SNJ)
iwl_trans->trans_cfg = &iwl_so_trans_cfg;
#if IS_ENABLED(CONFIG_IWLMVM)
--
2.29.2
Re: [PATCH v2] vio: make remove callback return void
On Thu, Feb 25, 2021 at 12:52 PM Michael Ellerman wrote: > > Uwe Kleine-König writes: > > The driver core ignores the return value of struct bus_type::remove() > > because there is only little that can be done. To simplify the quest to > > make this function return void, let struct vio_driver::remove() return > > void, too. All users already unconditionally return 0, this commit makes > > it obvious that returning an error code is a bad idea and makes it > > obvious for future driver authors that returning an error code isn't > > intended. > > > > Note there are two nominally different implementations for a vio bus: > > one in arch/sparc/kernel/vio.c and the other in > > arch/powerpc/platforms/pseries/vio.c. I didn't care to check which > > driver is using which of these busses (or if even some of them can be > > used with both) and simply adapt all drivers and the two bus codes in > > one go. > > I'm 99% sure there's no connection between the two implementations, > other than the name. > > So splitting the patch by arch would make it easier to merge. I'm > reluctant to merge changes to sparc code. The sparc subsystem clearly started out as a copy of the powerpc version, and serves roughly the same purpose, but the communication with the hypervisor is quite different. As there are only four drivers for the sparc vio subsystem: drivers/block/sunvdc.c drivers/net/ethernet/sun/ldmvsw.c drivers/net/ethernet/sun/sunvnet.c drivers/tty/vcc.c maybe it would make sense to rename those to use distinct identifiers now? Arnd
[PATCH] net/mlx5e: fix mlx5e_tc_tun_update_header_ipv6 dummy definition
From: Arnd Bergmann
The alternative implementation of this function in a header file
is declared as a global symbol, and gets added to every .c file
that includes it, which leads to a link error:
arm-linux-gnueabi-ld: drivers/net/ethernet/mellanox/mlx5/core/en_rx.o: in
function `mlx5e_tc_tun_update_header_ipv6':
en_rx.c:(.text+0x0): multiple definition of `mlx5e_tc_tun_update_header_ipv6';
drivers/net/ethernet/mellanox/mlx5/core/en_main.o:en_main.c:(.text+0x0): first
defined here
Mark it 'static inline' like the other functions here.
Fixes: c7b9038d8af6 ("net/mlx5e: TC preparation refactoring for routing update
event")
Signed-off-by: Arnd Bergmann
---
drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
index 67de2bf36861..89d5ca91566e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
@@ -76,10 +76,12 @@ int mlx5e_tc_tun_update_header_ipv6(struct mlx5e_priv *priv,
static inline int
mlx5e_tc_tun_create_header_ipv6(struct mlx5e_priv *priv,
struct net_device *mirred_dev,
- struct mlx5e_encap_entry *e) { return
-EOPNOTSUPP; }
-int mlx5e_tc_tun_update_header_ipv6(struct mlx5e_priv *priv,
- struct net_device *mirred_dev,
- struct mlx5e_encap_entry *e)
+ struct mlx5e_encap_entry *e)
+{ return -EOPNOTSUPP; }
+static inline int
+mlx5e_tc_tun_update_header_ipv6(struct mlx5e_priv *priv,
+ struct net_device *mirred_dev,
+ struct mlx5e_encap_entry *e)
{ return -EOPNOTSUPP; }
#endif
int mlx5e_tc_tun_route_lookup(struct mlx5e_priv *priv,
--
2.29.2
Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support
On Thu, Feb 18, 2021 at 4:28 AM Min Li wrote: > > If the driver can use the same algorithm that is in your user space software > > today, that would seem to be a nicer way to handle it than requiring a > > separate application. > > > > Hi Arnd > > > What is the device driver that you are referring here? > > In summary of your reviews, are you suggesting me to discard this change > and go back to PTP subsystem to find a better place for things that I wanna > do here? Yes, I mean doing it all in the PTP driver. Arnd
Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support
On Wed, Feb 17, 2021 at 9:20 PM Min Li wrote: > > I attached the G.8273.2 document, where chapter 6 is about supporting > physical layer > frequency. And combo mode is Renesas way to support this requirement. Other > companies > may come up with different ways to support it. > > When EEC quality is below certain level, we would wanna turn off combo mode. Maybe this is something that could be handled inside of the device driver then? If the driver can use the same algorithm that is in your user space software today, that would seem to be a nicer way to handle it than requiring a separate application. > > > This function will read FCW first and convert it to FFO. > > > > Is this related to the information in the timex->freq field? It sounds like > > this > > would already be accessible through the existing > > clock_adjtime() interface. > > > > They are related, but dealing with timex->freq has limitations > > 1) Renesas SMU has up to 8 DPLLs and only one of the them would be ptp > clock and we want to be able to read any DPLL's FFO or state Is this necessarily unique to Renesas SMU though? Not sure what makes sense in terms of the phc/ptp interface. Could there just be a separate instance for each DPLL in the phc subsystem even if it's actually a ptp clock, or would that be an incorrect use? > 2) timex->freq's unit is ppb and we want to read more precise ffo in smaller > unit of ppqt This also sounds like something that would not be vendor specific. If you need a higher resolution, then at some point others would need it as well. There is already precedence in 'struct timex' to redefine the resolution of some fields based on a flag -- 'time.tv_usec' can either refer to microseconds to nanoseconds. If the range of the 'freq' field is sufficient to encode ppqt, you could add another flag for that, otherwise another reserved field can be used. > 3) there is no interface in the current ptp hardware clock infrastructure to > read ffo back from hardware Adding an internal interface is the easy part here, the hard part is defining the user interface. > > Wouldn't any PTP clock run in one of these modes? If this is just > > informational, it might be appropriate to have another sysfs attribute for > > each PTP clock that shows the state of the DPLL, and then have the PTP > > driver either fill in the current value in 'struct ptp_clock', or provide a > > callback to report the state when a user reads the sysfs attribute. > > > > What you propose can work. But DPLL operating mode is not standardized > so different vendor may have different explanation for various modes. If it's a string, that could easily be extended to further modes, as long as the kernel documents which names are allowed. If multiple vendors refer to the same mode by different names, someone will have to decide what to call it in the kernel, and everyone afterwards would use the same name. > Also, I thought sysfs is only for debug or informational purpose. > Production software is not supposed to use it for critical tasks? No, you are probably thinking of debugfs. sysfs is one of multiple common ways to exchange this kind of data in a reliable way. An ioctl would probably work just as well though, usually sysfs is better when the information makes sense to human operators or simple shell scripts, while an ioctl interface is better if performance is important, or if the information is primarily used in C programs. Arnd
Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support
On Tue, Feb 16, 2021 at 11:14 PM Min Li wrote: > > I can't help but think you are evading my question I asked. If there is no > > specific action that this pcm4l tool needs to perform, then I'd think we > > should better not provide any interface for it at all. > > > > I also found a reference to only closed source software at > > https://www.renesas.com/us/en/software-tool/ptp-clock-manager-linux > > We don't add low-level interfaces to the kernel that are only usable by > > closed-source software. > > > > Once you are able to describe the requirements for what pcm4l actually > > needs from the hardware, we can start discussing what a high-level > > interface would look like that can be used to replace the your current > > interface, in a way that would work across vendors and with both pcm4l and > > open-source tools that do the same job. > > This driver is used by pcm4l to access functionalities that cannot be > accessed through PHC(ptp hardware clock) interface. > > All these functions are kind of specific to Renesas SMU device and I have > never heard other devices offering similar functions > > The 3 functions currently provided are (more to be added in the future) > > - set combomode > > In Telecom Boundary Clock (T-BC) and Telecom Time Slave Clock (T-TSC) > applications > per ITU-T G.8275.2, two DPLLs can be used: > one DPLL is configured as a DCO to synthesize PTP clocks, and the other DPLL > is > configured as an EEC(Ethernet Equipment Clock) to generate physical layer > clocks. > Combo mode provides physical layer frequency support from the EEC/SEC to the > PTP > clock. Thank you for the explanation. Now, to take the question to an even higher level, is it useful to leave it up to the user to pick one of the two modes explicitly, or can the kernel make that decision based on some other information that it already has, or that can be supplied to it using a more abstract interface? In other words, when would a user pick combomode over non-combomode or vice versa? Would it make sense to have this configured according to the hardware platform, e.g. in a device tree property of the device, rather than having the user choose a mode? Which of the two possible modes do other PTP devices use that support DCO and EEC but are not configurable? > - read DPLL's FFO > > Read fractional frequency offset (FFO) from a DPLL. > > For a DPLL channel, a Frequency Control Word (FCW) is used to adjust the > frequency output of the DCO. A positive value will increase the output > frequency > and a negative one will decrease the output frequency. > > This function will read FCW first and convert it to FFO. Is this related to the information in the timex->freq field? It sounds like this would already be accessible through the existing clock_adjtime() interface. > -read DPLL's state > > The DPLLs support four primary operating modes: Free-Run, Locked, > Holdover, and DCO. In Free-Run mode the DPLLs synthesize clocks > based on the system clock alone. In Locked mode the DPLLs filter > reference clock jitter with the selected bandwidth. Additionally in > Locked mode, the long-term output frequency accuracy is the same > as the long-term frequency accuracy of the selected input reference. > In Holdover mode, the DPLL uses frequency data acquired while in > Locked mode to generate accurate frequencies when input > references are not available. In DCO mode, the DPLL control loop > is opened and the DCO can be controlled by a PTP clock recovery > servo running on an external processor to synthesize PTP clocks. Wouldn't any PTP clock run in one of these modes? If this is just informational, it might be appropriate to have another sysfs attribute for each PTP clock that shows the state of the DPLL, and then have the PTP driver either fill in the current value in 'struct ptp_clock', or provide a callback to report the state when a user reads the sysfs attribute. Arnd
Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support
On Tue, Feb 16, 2021 at 6:10 PM Min Li wrote: > > > If I come up with a new file and move all the abstraction code there, > > > does that work? > > > > I think so, but it's more important to figure out a good user space > > interface > > first. The ioctl interfaces should be written on a higher-level > > abstraction, to > > ensure they can work with any hardware implementation and are not > > specific to Renesas devices. > > > > Can you describe on an abstract level how a user would use the character > > device, and what they achieve by that? > > This driver is meant to be used by Renesas PTP Clock Manager for > Linux (pcm4l) software for Renesas device only. > > About how pcm4l uses the char device, pcm4l will open the device > and do the supported ioctl cmds on the device, simple like that. > > At the same time, pcm4l will also open ptp hardware clock device, > which is /dev/ptp[x], to do clock adjustments. I can't help but think you are evading my question I asked. If there is no specific action that this pcm4l tool needs to perform, then I'd think we should better not provide any interface for it at all. I also found a reference to only closed source software at https://www.renesas.com/us/en/software-tool/ptp-clock-manager-linux We don't add low-level interfaces to the kernel that are only usable by closed-source software. Once you are able to describe the requirements for what pcm4l actually needs from the hardware, we can start discussing what a high-level interface would look like that can be used to replace the your current interface, in a way that would work across vendors and with both pcm4l and open-source tools that do the same job. Arnd
Re: linux-next: manual merge of the net-next tree with the arm-soc tree
On Tue, Feb 16, 2021 at 3:20 AM wrote: > > > > I fixed it up (see below) and can carry the fix as necessary. This > > is now fixed as far as linux-next is concerned, but any non trivial > > conflicts should be mentioned to your upstream maintainer when your tree > > is submitted for merging. You may also want to consider cooperating > > with the maintainer of the conflicting tree to minimise any particularly > > complex conflicts. > > > > This is because the DTS changes are included in net-next. This patch should > be merged via the soc tree. > I had the same problem before. How is it correct to send a DTS patch? > Should I separate into different series? I have already sent the pull requests for the dts files to Linus, so that's not changing any more for this time, and he will just have to fix it up when he pulls both branches. In the future, please send all dts updates to s...@kernel.org (after the binding and driver is merged) rather than together with the device drivers. Sending the devicetree binding updates is a little trickier, as we tend to want them merged both with the driver and the dts files. One way to do this is to have a shared branch for the bindings updates, and then base both the driver branch and the dts branch on top of the same commits for that. A simpler alternative is to merge only the driver and binding changes in one release, and send the dts changes for the following release. This obviously takes longer to complete. Arnd
Re: [PATCH 2/4] net: stmmac: Add Toshiba Visconti SoCs glue driver
On Mon, Feb 15, 2021 at 10:23 AM Leon Romanovsky wrote: > On Mon, Feb 15, 2021 at 04:28:09PM +0900, Nobuhiro Iwamatsu wrote: > > > > Sorry, I sent the wrong patchset that didn't fix this point out. > > > > > I asked it before, but never received an answer. > > > > I have received your point out and have sent an email with the content > > to remove this line. But it may not have arrived yet... > > > > > Why did you use "def_bool y" and not "default y"? Isn't it supposed to be > > > "depends on STMMAC_ETH"? And probably it shouldn't be set as a default as > > > "y". > > > > > > > The reason why "def_bool y" was set is that the wrong fix was left when > > debugging. Also, I don't think it is necessary to set "default y". > > This is also incorrect because it says "bool" Toshiba Visconti DWMAC > > support "". I change it to trustate in the new patch. > > > > And this driver is enabled when STMMAC_PLATFORM was Y. And STMMAC_PLATFORM > > depends on STMMAC_ETH. > > So I understand that STMMAC_ETH does not need to be dependents. Is this > > understanding wrong? > > This is correct understanding, just need to clean other entries in that > Kconfig that depends on STMMAC_ETH. 'tristate' with no default sounds right. I see that some platforms have a default according to the platform, which also makes sense but isn't required. What I would suggest though is a dependency on the platform, to make it easier to disable the front-end based on which platforms are enabled. This would end up as config DWMAC_VISCONTI tristate "Toshiba Visconti DWMAC support" depends on ARCH_VISCONTI || COMPILE_TEST depends on OF && COMMON_CLK # only add this line if it's required for compilation default ARCH_VISCONTI Arnd
Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support
On Fri, Feb 12, 2021 at 5:19 PM Min Li wrote: > > > > > Ah, so if this is for a PTP related driver, it should probably be > > integrated into > > the PTP subsystem rather than being a separate class. > > > > I was trying to add these functions to PHC subsystem but was not accepted > because the functions > are specific to Renesas device and there is no place for those functions in > PHC driver. It would be useful to explain that in the patch description and link to the original discussion there. What exactly was the objection? > > > > This tells me that you got the abstraction the wrong way: the common > > > > files should not need to know anything about the specific > > implementations. > > > > > > > > Instead, these should be in separate modules that call exported > > > > functions from the common code. > > > > > > > > > > > > > > I got what you mean. But so far it only supports small set of > > > functions, which is why I don't feet it is worth the effort to over > > > abstract > > things. > > > > Then maybe pick one of the two hardware variants and drop the abstraction > > you have. You can then add more features before you add a proper > > abstraction layer and then the second driver. > > > > If I come up with a new file and move all the abstraction code there, > does that work? I think so, but it's more important to figure out a good user space interface first. The ioctl interfaces should be written on a higher-level abstraction, to ensure they can work with any hardware implementation and are not specific to Renesas devices. Can you describe on an abstract level how a user would use the character device, and what they achieve by that? Arnd
Re: [PATCH v2 4/4] arm: dts: visconti: Add DT support for Toshiba Visconti5 ethernet controller
On Fri, Feb 12, 2021 at 4:03 AM Nobuhiro Iwamatsu
wrote:
> @@ -384,6 +398,16 @@ spi6: spi@28146000 {
> #size-cells = <0>;
> status = "disabled";
> };
> +
> + piether: ethernet@2800 {
> + compatible = "toshiba,visconti-dwmac";
Shouldn't there be a more specific compatible string here, as well as the
particular version of the dwmac you use?
In the binding example, you list the device as "dma-coherent",
but in this instance, it is not marked that way. Can you find out
whether the device is in fact connected properly to a cache-coherent
bus?
Note that failing to mark it as cache-coherent will make the device
rather slow and possibly not work correctly if it is in fact coherent,
but the default is non-coherent since a lot of SoCs are lacking
that hardware support.
Arnd
Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support
On Fri, Feb 12, 2021 at 2:40 AM Min Li wrote: > > There should probably be a description of the purpose of the hardware both > > here and in the patch description. > > > > In particular, please explain how it relates to the existing clockmatrix > > driver. > > I just uploaded v2 patch to provide more background info for this change. It appears that you accidentally dropped the Cc list, adding them back in the reply. Also adding the PTP maintainer, as this clearly needs to be reviewed in the context of the PTP subsystem. Please keep Richard and the netdev list on Cc in future submissions. > This driver is developed to be only used by Renesas PTP Clock Manager for > Linux (pcm4l) software. > > This driver supports 1588/PTP releated functionalities that > specific to Renesas devices but are not supported by general PHC framework. > So pcm4l will use both the existing PHC driver and this driver to complete > 1588/PTP support. Ah, so if this is for a PTP related driver, it should probably be integrated into the PTP subsystem rather than being a separate class. > > A pure list of register values seems neither particular portable nor > > intuitive. > > How is a user expected to interpret these, and are you sure that any driver > > for this class would have the same interpretation at the same register > > index? > > > > Yes we need a way to dump register values when remote debugging with > customers. > And all the Renesas SMU has similar register layout A sysfs interface is a poor choice for this though -- how can you guarantee that even future Renesas devices follow the exact same register layout? By encoding the current hardware generation into the user interface, you would end up having to emulate this on other chips you want to support later. If it's only for debugging, best leave it out of the public interface, and only have it in your own copy of the driver until the bugs are gone, or add a debugfs interface. > > Can you explain the purpose of this restriction? Why is it ok for two > > threads > > to access the same file descriptor, but not two different file descriptors > > for > > the same device? > > > > The mutex is there to provide synchronization to the device access while PHC > driver is accessing the device. > The atomic count is to make sure only one user space program is using the > driver at a time. Then remove the atomic count, as it clearly doesn't do what you describe when you can have multiple threads access the driver concurrently. > > Each of these needs a device tree binding. It's usually better to name the > > compatible strings according to the chips that contain this hardware, such > > as > > "renesas,r8a1234567-rsmucdev" instead of "renesas,rsmu-cdev0". > > > > Since you don't seem to about the difference between the devices, the driver > > can also just bind to one of them (usually the oldest) and then the newer > > devices contain the string as a fallback, so you don't have to update the > > driver every time another variant gets made. > > > > > > Actually the device is not spawned from device tree but from Renesas MFD > driver (submitted in a separate thread). > The MFD driver will call mfd_add_devices to create the platform devices. I am > not sure if I still need to create binding > In this case. If you have an of_device_id table, it needs a binding. It sounds like you don't need the of_device_id though. > > This should probably be part of the .c file, as no other driver needs to > > interface with it. > > > > We actually run a unit test on the driver that needs to access this structure. > That is why I need to put it in a header Unit tests are good, but it's better to have them in the kernel. Can you add the unit test into the patch then? We now have the kunit framework for running unit tests. > > This tells me that you got the abstraction the wrong way: the common files > > should not need to know anything about the specific implementations. > > > > Instead, these should be in separate modules that call exported functions > > from the common code. > > > > > > I got what you mean. But so far it only supports small set of functions, > which is why > I don't feet it is worth the effort to over abstract things. Then maybe pick one of the two hardware variants and drop the abstraction you have. You can then add more features before you add a proper abstraction layer and then the second driver. Arnd
[PATCH] carl9170: fix struct alignment conflict
From: Arnd Bergmann
Multiple structures in the carl9170 driver have alignment
impossible alignment constraints that gcc warns about when
building with 'make W=1':
drivers/net/wireless/ath/carl9170/fwcmd.h:243:2: warning: alignment 1 of 'union
' is less than 4 [-Wpacked-not-aligned]
drivers/net/wireless/ath/carl9170/wlan.h:373:1: warning: alignment 1 of 'struct
ar9170_rx_frame_single' is less than 2 [-Wpacked-not-aligned]
In the carl9170_cmd structure, multiple members that have an explicit
alignment requirement of four bytes are added into a union with explicit
byte alignment, but this in turn is part of a structure that also has
four-byte alignment.
In the wlan.h header, multiple structures contain a ieee80211_hdr member
that is required to be two-byte aligned to avoid alignmnet faults when
processing network headers, but all members are forced to be byte-aligned
using the __packed tag at the end of the struct definition.
In both cases, leaving out the packing does not change the internal
layout of the structure but changes the alignment constraint of the
structure itself.
Change all affected structures to only apply packing where it does
not violate the alignment requirement of the contained structure.
Signed-off-by: Arnd Bergmann
---
drivers/net/wireless/ath/carl9170/fwcmd.h | 2 +-
drivers/net/wireless/ath/carl9170/wlan.h | 20 ++--
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/net/wireless/ath/carl9170/fwcmd.h
b/drivers/net/wireless/ath/carl9170/fwcmd.h
index 56999a3b9d3b..4a50009c 100644
--- a/drivers/net/wireless/ath/carl9170/fwcmd.h
+++ b/drivers/net/wireless/ath/carl9170/fwcmd.h
@@ -240,7 +240,7 @@ struct carl9170_cmd {
struct carl9170_bcn_ctrl_cmdbcn_ctrl;
struct carl9170_rx_filter_cmd rx_filter;
u8 data[CARL9170_MAX_CMD_PAYLOAD_LEN];
- } __packed;
+ } __packed __aligned(4);
} __packed __aligned(4);
#defineCARL9170_TX_STATUS_QUEUE3
diff --git a/drivers/net/wireless/ath/carl9170/wlan.h
b/drivers/net/wireless/ath/carl9170/wlan.h
index ea17995b32f4..bb73553fd7c2 100644
--- a/drivers/net/wireless/ath/carl9170/wlan.h
+++ b/drivers/net/wireless/ath/carl9170/wlan.h
@@ -367,27 +367,27 @@ struct ar9170_rx_macstatus {
struct ar9170_rx_frame_single {
struct ar9170_rx_head phy_head;
- struct ieee80211_hdr i3e;
+ struct ieee80211_hdr i3e __packed __aligned(2);
struct ar9170_rx_phystatus phy_tail;
struct ar9170_rx_macstatus macstatus;
-} __packed;
+};
struct ar9170_rx_frame_head {
struct ar9170_rx_head phy_head;
- struct ieee80211_hdr i3e;
+ struct ieee80211_hdr i3e __packed __aligned(2);
struct ar9170_rx_macstatus macstatus;
-} __packed;
+};
struct ar9170_rx_frame_middle {
- struct ieee80211_hdr i3e;
+ struct ieee80211_hdr i3e __packed __aligned(2);
struct ar9170_rx_macstatus macstatus;
-} __packed;
+};
struct ar9170_rx_frame_tail {
- struct ieee80211_hdr i3e;
+ struct ieee80211_hdr i3e __packed __aligned(2);
struct ar9170_rx_phystatus phy_tail;
struct ar9170_rx_macstatus macstatus;
-} __packed;
+};
struct ar9170_rx_frame {
union {
@@ -395,8 +395,8 @@ struct ar9170_rx_frame {
struct ar9170_rx_frame_head head;
struct ar9170_rx_frame_middle middle;
struct ar9170_rx_frame_tail tail;
- } __packed;
-} __packed;
+ };
+};
static inline u8 ar9170_get_decrypt_type(struct ar9170_rx_macstatus *t)
{
--
2.29.2
[PATCH] cwmwl8k: fix alignment constraints
From: Arnd Bergmann
sturct mwl8k_dma_data contains a ieee80211_hdr structure, which is required to
have at least two byte alignment, and this conflicts with the __packed
attribute:
vers/net/wireless/marvell/mwl8k.c:811:1: warning: alignment 1 of 'struct
mwl8k_dma_data' is less than 2 [-Wpacked-not-aligned]
Mark mwl8k_dma_data itself as having two-byte alignment to ensure the
inner structure is properly aligned.
Signed-off-by: Arnd Bergmann
---
drivers/net/wireless/marvell/mwl8k.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/marvell/mwl8k.c
b/drivers/net/wireless/marvell/mwl8k.c
index abf3b0233ccc..38eeab6369f7 100644
--- a/drivers/net/wireless/marvell/mwl8k.c
+++ b/drivers/net/wireless/marvell/mwl8k.c
@@ -808,7 +808,7 @@ struct mwl8k_dma_data {
__le16 fwlen;
struct ieee80211_hdr wh;
char data[];
-} __packed;
+} __packed __aligned(2);
/* Routines to add/remove DMA header from skb. */
static inline void mwl8k_remove_dma_header(struct sk_buff *skb, __le16 qos)
--
2.29.2
[PATCH] brcm80211: fix alignment constraints
From: Arnd Bergmann
sturct d11txh contains a ieee80211_rts structure, which is required to
have at least two byte alignment, and this conflicts with the __packed
attribute:
drivers/net/wireless/broadcom/brcm80211/brcmsmac/d11.h:786:1: warning:
alignment 1 of 'struct d11txh' is less than 2 [-Wpacked-not-aligned]
Mark d11txh itself as having two-byte alignment to ensure the
inner structure is properly aligned.
Signed-off-by: Arnd Bergmann
---
drivers/net/wireless/broadcom/brcm80211/brcmsmac/d11.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/d11.h
b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/d11.h
index 9035cc4d6ff3..7870093629c3 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/d11.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/d11.h
@@ -783,7 +783,7 @@ struct d11txh {
u8 RTSPhyHeader[D11_PHY_HDR_LEN]; /* 0x2c - 0x2e */
struct ieee80211_rts rts_frame; /* 0x2f - 0x36 */
u16 PAD;/* 0x37 */
-} __packed;
+} __packed __aligned(2);
#defineD11_TXH_LEN 112 /* bytes */
--
2.29.2
[PATCH] wl3501: fix alignment constraints
From: Arnd Bergmann
struct wl3501_80211_tx_hdr contains a ieee80211_hdr structure, which is
required to have at least two byte alignment, and this conflicts with
the __packed attribute:
wireless/wl3501.h:553:1: warning: alignment 1 of 'struct wl3501_80211_tx_hdr'
is less than 2 [-Wpacked-not-aligned]
Mark wl3501_80211_tx_hdr itself as having two-byte alignment to ensure the
inner structure is properly aligned.
Signed-off-by: Arnd Bergmann
---
drivers/net/wireless/wl3501.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/wl3501.h b/drivers/net/wireless/wl3501.h
index b446cb369557..e98e04ee9a2c 100644
--- a/drivers/net/wireless/wl3501.h
+++ b/drivers/net/wireless/wl3501.h
@@ -550,7 +550,7 @@ struct wl3501_80211_tx_plcp_hdr {
struct wl3501_80211_tx_hdr {
struct wl3501_80211_tx_plcp_hdr pclp_hdr;
struct ieee80211_hdrmac_hdr;
-} __packed;
+} __packed __aligned(2);
/*
Reserve the beginning Tx space for descriptor use.
--
2.29.2
[PATCH] can: ucan: fix alignment constraints
From: Arnd Bergmann
struct ucan_message_in contains member with 4-byte alignment
but is itself marked as unaligned, which triggers a warning:
drivers/net/can/usb/ucan.c:249:1: warning: alignment 1 of 'struct
ucan_message_in' is less than 4 [-Wpacked-not-aligned]
Mark the outer structure to have the same alignment as the inner
one.
Signed-off-by: Arnd Bergmann
---
drivers/net/can/usb/ucan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/can/usb/ucan.c b/drivers/net/can/usb/ucan.c
index fa403c080871..536c985dad21 100644
--- a/drivers/net/can/usb/ucan.c
+++ b/drivers/net/can/usb/ucan.c
@@ -246,7 +246,7 @@ struct ucan_message_in {
*/
struct ucan_tx_complete_entry_t can_tx_complete_msg[0];
} __aligned(0x4) msg;
-} __packed;
+} __packed __aligned(0x4);
/* Macros to calculate message lengths */
#define UCAN_OUT_HDR_SIZE offsetof(struct ucan_message_out, msg)
--
2.29.2
[PATCH] net: hns3: avoid -Wpointer-bool-conversion warning
From: Arnd Bergmann
clang points out a redundant sanity check:
drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c:497:28: error: address of
array 'filp->f_path.dentry->d_iname' will always evaluate to 'true'
[-Werror,-Wpointer-bool-conversion]
This can never fail, so just remove the check.
Fixes: 04987ca1b9b6 ("net: hns3: add debugfs support for tm nodes, priority and
qset info")
Signed-off-by: Arnd Bergmann
---
drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
index 6978304f1ac5..c5958754f939 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
@@ -494,9 +494,6 @@ static ssize_t hns3_dbg_read(struct file *filp, char __user
*buffer,
ssize_t size = 0;
int ret = 0;
- if (!filp->f_path.dentry->d_iname)
- return -EINVAL;
-
read_buf = kzalloc(HNS3_DBG_READ_LEN, GFP_KERNEL);
if (!read_buf)
return -ENOMEM;
--
2.29.2
Re: [PATCH net-next] net: dsa: hellcreek: Add missing TAPRIO dependency
On Thu, Jan 28, 2021 at 5:37 PM Vladimir Oltean wrote:
>
> On Thu, Jan 28, 2021 at 05:33:38PM +0100, Kurt Kanzenbach wrote:
> > Add missing dependency to TAPRIO to avoid build failures such as:
> >
> > |ERROR: modpost: "taprio_offload_get"
> > [drivers/net/dsa/hirschmann/hellcreek_sw.ko] undefined!
> > |ERROR: modpost: "taprio_offload_free"
> > [drivers/net/dsa/hirschmann/hellcreek_sw.ko] undefined!
> >
> > Fixes: 24dfc6eb39b2 ("net: dsa: hellcreek: Add TAPRIO offloading support")
> > Reported-by: Randy Dunlap
> > Signed-off-by: Kurt Kanzenbach
Acked-by: Arnd Bergmann
> Note that for sja1105, Arnd solved it this way. I am still not sure why.
>
> commit 5d294fc483405de9c0913ab744a31e6fa7cb0f40
> Author: Arnd Bergmann
> Date: Fri Oct 25 09:26:35 2019 +0200
>
> net: dsa: sja1105: improve NET_DSA_SJA1105_TAS dependency
>
> An earlier bugfix introduced a dependency on CONFIG_NET_SCH_TAPRIO,
> but this missed the case of NET_SCH_TAPRIO=m and NET_DSA_SJA1105=y,
> which still causes a link error:
>
As I described in this commit, the problem here was that NET_DSA_SJA1105_TAS
is a 'bool' symbol with a dependency on a 'tristate', so you have to prevent
the option from getting enabled when it's part of a driver that gets built into
the kernel but the dependnecy is in a loadable module.
NET_DSA_HIRSCHMANN_HELLCREEK on the other hand is a 'tristate'
symbol itself, so the dependency takes care of it: you cannot set it to =y
when its dependency is =m.
Arnd
[PATCH] ath9k: fix build error with LEDS_CLASS=m
From: Arnd Bergmann
When CONFIG_ATH9K is built-in but LED support is in a loadable
module, both ath9k drivers fails to link:
x86_64-linux-ld: drivers/net/wireless/ath/ath9k/gpio.o: in function
`ath_deinit_leds':
gpio.c:(.text+0x36): undefined reference to `led_classdev_unregister'
x86_64-linux-ld: drivers/net/wireless/ath/ath9k/gpio.o: in function
`ath_init_leds':
gpio.c:(.text+0x179): undefined reference to `led_classdev_register_ext'
The problem is that the 'imply' keyword does not enforce any dependency
but is only a weak hint to Kconfig to enable another symbol from a
defconfig file.
Change imply to a 'depends on LEDS_CLASS' that prevents the incorrect
configuration but still allows building the driver without LED support.
The 'select MAC80211_LEDS' is now ensures that the LED support is
actually used if it is present, and the added Kconfig dependency
on MAC80211_LEDS ensures that it cannot be enabled manually when it
has no effect.
Fixes: 197f466e93f5 ("ath9k_htc: Do not select MAC80211_LEDS by default")
Signed-off-by: Arnd Bergmann
---
drivers/net/wireless/ath/ath9k/Kconfig | 8 ++--
net/mac80211/Kconfig | 2 +-
2 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/Kconfig
b/drivers/net/wireless/ath/ath9k/Kconfig
index a84bb9b6573f..e150d82eddb6 100644
--- a/drivers/net/wireless/ath/ath9k/Kconfig
+++ b/drivers/net/wireless/ath/ath9k/Kconfig
@@ -21,11 +21,9 @@ config ATH9K_BTCOEX_SUPPORT
config ATH9K
tristate "Atheros 802.11n wireless cards support"
depends on MAC80211 && HAS_DMA
+ select MAC80211_LEDS if LEDS_CLASS=y || LEDS_CLASS=MAC80211
select ATH9K_HW
select ATH9K_COMMON
- imply NEW_LEDS
- imply LEDS_CLASS
- imply MAC80211_LEDS
help
This module adds support for wireless adapters based on
Atheros IEEE 802.11n AR5008, AR9001 and AR9002 family
@@ -176,11 +174,9 @@ config ATH9K_PCI_NO_EEPROM
config ATH9K_HTC
tristate "Atheros HTC based wireless cards support"
depends on USB && MAC80211
+ select MAC80211_LEDS if LEDS_CLASS=y || LEDS_CLASS=MAC80211
select ATH9K_HW
select ATH9K_COMMON
- imply NEW_LEDS
- imply LEDS_CLASS
- imply MAC80211_LEDS
help
Support for Atheros HTC based cards.
Chipsets supported: AR9271
diff --git a/net/mac80211/Kconfig b/net/mac80211/Kconfig
index cd9a9bd242ba..51ec8256b7fa 100644
--- a/net/mac80211/Kconfig
+++ b/net/mac80211/Kconfig
@@ -69,7 +69,7 @@ config MAC80211_MESH
config MAC80211_LEDS
bool "Enable LED triggers"
depends on MAC80211
- depends on LEDS_CLASS
+ depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
select LEDS_TRIGGERS
help
This option enables a few LED triggers for different
--
2.29.2
[PATCH] [net-next] ipa: add remoteproc dependency
From: Arnd Bergmann
Compile-testing without CONFIG_REMOTEPROC results in a build failure:
>>> referenced by ipa_main.c
>>> net/ipa/ipa_main.o:(ipa_probe) in archive drivers/built-in.a
ld.lld: error: undefined symbol: rproc_put
>>> referenced by ipa_main.c
>>> net/ipa/ipa_main.o:(ipa_probe) in archive drivers/built-in.a
>>> referenced by ipa_main.c
>>> net/ipa/ipa_main.o:(ipa_remove) in archive drivers/built-in.a
Add a new dependency to avoid this.
Fixes: 38a4066f593c ("net: ipa: support COMPILE_TEST")
Signed-off-by: Arnd Bergmann
---
drivers/net/ipa/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ipa/Kconfig b/drivers/net/ipa/Kconfig
index b68f1289b89e..aa1c0ae3cf01 100644
--- a/drivers/net/ipa/Kconfig
+++ b/drivers/net/ipa/Kconfig
@@ -3,6 +3,7 @@ config QCOM_IPA
depends on 64BIT && NET && QCOM_SMEM
depends on ARCH_QCOM || COMPILE_TEST
depends on QCOM_RPROC_COMMON || (QCOM_RPROC_COMMON=n && COMPILE_TEST)
+ depends on REMOTEPROC
select QCOM_MDT_LOADER if ARCH_QCOM
select QCOM_QMI_HELPERS
help
--
2.29.2
[PATCH] [net-next] bonding: add TLS dependency
From: Arnd Bergmann When TLS is a module, the built-in bonding driver may cause a link error: x86_64-linux-ld: drivers/net/bonding/bond_main.o: in function `bond_start_xmit': bond_main.c:(.text+0xc451): undefined reference to `tls_validate_xmit_skb' Add a dependency to avoid the problem. Signed-off-by: Arnd Bergmann --- I could not figure out when this started, it seems to have been possible for a while. --- drivers/net/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig index 672fcdd9aecb..45d12b0e9a2f 100644 --- a/drivers/net/Kconfig +++ b/drivers/net/Kconfig @@ -42,6 +42,7 @@ config BONDING tristate "Bonding driver support" depends on INET depends on IPV6 || IPV6=n + depends on TLS || TLS_DEVICE=n help Say 'Y' or 'M' if you wish to be able to 'bond' multiple Ethernet Channels together. This is called 'Etherchannel' by Cisco, -- 2.29.2
Re: [PATCH] ath9k: fix build error with LEDS_CLASS=m
On Mon, Jan 25, 2021 at 12:40 PM Krzysztof Kozlowski wrote: > On Mon, 25 Jan 2021 at 12:36, Arnd Bergmann wrote: > > > > From: Arnd Bergmann > > > > When CONFIG_ATH9K is built-in but LED support is in a loadable > > module, both ath9k drivers fails to link: > > > > x86_64-linux-ld: drivers/net/wireless/ath/ath9k/gpio.o: in function > > `ath_deinit_leds': > > gpio.c:(.text+0x36): undefined reference to `led_classdev_unregister' > > x86_64-linux-ld: drivers/net/wireless/ath/ath9k/gpio.o: in function > > `ath_init_leds': > > gpio.c:(.text+0x179): undefined reference to `led_classdev_register_ext' > > > > The problem is that the 'imply' keyword does not enforce any dependency > > but is only a weak hint to Kconfig to enable another symbol from a > > defconfig file. > > > > Change imply to a 'depends on LEDS_CLASS' that prevents the incorrect > > configuration but still allows building the driver without LED support. > > > > The 'select MAC80211_LEDS' is now ensures that the LED support is > > actually used if it is present, and the added Kconfig dependency > > on MAC80211_LEDS ensures that it cannot be enabled manually when it > > has no effect. > > But we do not want to have this dependency (selecting MAC80211_LEDS). > I fixed this problem here: > https://lore.kernel.org/lkml/20201227143034.1134829-1-k...@kernel.org/ > Maybe let's take this approach? Generally speaking, I don't like to have a device driver specific Kconfig setting 'select' a subsystem', for two reasons: - you suddenly get asked for tons of new LED specific options when enabling seemingly benign options - Mixing 'depends on' and 'select' leads to bugs with circular dependencies that usually require turning some other 'select' into 'depends on'. The problem with LEDS_CLASS in particular is that there is a mix of drivers using one vs the other roughly 50:50. Arnd
Re: [PATCH] [net-next] ipa: add remoteproc dependency
On Mon, Jan 25, 2021 at 4:23 PM Bjorn Andersson
wrote:
>
> On Mon 25 Jan 05:35 CST 2021, Arnd Bergmann wrote:
>
> > From: Arnd Bergmann
> >
> > Compile-testing without CONFIG_REMOTEPROC results in a build failure:
> >
> > >>> referenced by ipa_main.c
> > >>> net/ipa/ipa_main.o:(ipa_probe) in archive
> > >>> drivers/built-in.a
> > ld.lld: error: undefined symbol: rproc_put
> > >>> referenced by ipa_main.c
> > >>> net/ipa/ipa_main.o:(ipa_probe) in archive
> > >>> drivers/built-in.a
> > >>> referenced by ipa_main.c
> > >>> net/ipa/ipa_main.o:(ipa_remove) in archive
> > >>> drivers/built-in.a
> >
> > Add a new dependency to avoid this.
> >
>
> Afaict this should be addressed by:
>
> 86fdf1fc60e9 ("net: ipa: remove a remoteproc dependency")
>
> which is present in linux-next.
Ok, good. I was testing with next-20210122, which was
still lacking that commit.
Arnd
Re: [PATCH] ath9k: fix build error with LEDS_CLASS=m
On Mon, Jan 25, 2021 at 4:04 PM Krzysztof Kozlowski wrote: > On Mon, 25 Jan 2021 at 15:38, Arnd Bergmann wrote: > > On Mon, Jan 25, 2021 at 2:27 PM Krzysztof Kozlowski wrote: > > I meant that having MAC80211_LEDS selected causes the ath9k driver to > toggle on/off the WiFi LED. Every second, regardless whether it's > doing something or not. In my setup, I have problems with a WiFi > dongle somehow crashing (WiFi disappears, nothing comes from the > dongle... maybe it's Atheros FW, maybe some HW problem) and I found > this LED on/off slightly increases the chances of this dongle-crash. > That was the actual reason behind my commits. > > Second reason is that I don't want to send USB commands every second > when the device is idle. It unnecessarily consumes power on my > low-power device. Ok, I see. > Of course another solution is to just disable the trigger via sysfs > LED API. It would also work but my patch allows entire code to be > compiled-out (which was conditional in ath9k already). > > Therefore the patch I sent allows the ath9k LED option to be fully > choosable. Someone wants every-second-LED-blink, sure, enable > ATH9K_LEDS and you have it. Someone wants to reduce the kernel size, > don't enable ATH9K_LEDS. Originally, I think this is what CONFIG_MAC80211_LEDS was meant for, but it seems that this is not actually practical, since this also gets selected by half of the drivers using it, while the other half have a dependency on it. Out of the ones that select it, some in turn select LEDS_CLASS, while some depend on it. I think this needs a larger-scale cleanup for consistency between (at least) all the wireless drivers using LEDs. Either your patch or mine should get applied in the meantime, and I don't care much which one in this case, as we still have the remaining inconsistency. Arnd
Re: [PATCH] ath9k: fix build error with LEDS_CLASS=m
On Mon, Jan 25, 2021 at 2:27 PM Krzysztof Kozlowski wrote: > On Mon, 25 Jan 2021 at 14:09, Arnd Bergmann wrote: > > On Mon, Jan 25, 2021 at 12:40 PM Krzysztof Kozlowski > > wrote: > > > On Mon, 25 Jan 2021 at 12:36, Arnd Bergmann wrote: > > > But we do not want to have this dependency (selecting MAC80211_LEDS). > > > I fixed this problem here: > > > https://lore.kernel.org/lkml/20201227143034.1134829-1-k...@kernel.org/ > > > Maybe let's take this approach? > > > > Generally speaking, I don't like to have a device driver specific Kconfig > > setting 'select' a subsystem', for two reasons: > > > > - you suddenly get asked for tons of new LED specific options when > > enabling seemingly benign options > > > > - Mixing 'depends on' and 'select' leads to bugs with circular > > dependencies that usually require turning some other 'select' > > into 'depends on'. > > > > The problem with LEDS_CLASS in particular is that there is a mix of drivers > > using one vs the other roughly 50:50. > > Yes, you are right, I also don't like it. However it was like this > before my commit so I am not introducing a new issue. The point is > that in your choice the MAC80211_LEDS will be selected if LEDS_CLASS > is present, which is exactly what I was trying to fix/remove. My WiFi > dongle does not have a LED and it causes a periodic (every second) > event. However I still have LEDS_CLASS for other LEDS in the system. What is the effect of this lost event every second? If it causes some runtime warning or other problem, then neither of our fixes would solve it completely, because someone with a distro kernel would see the same issue when they have the symbol enabled but no physical LED in the device. Arnd
[PATCH] [5.8 regression] net: ks8851: fix link error
From: Arnd Bergmann
An object file cannot be built for both loadable module and built-in
use at the same time:
arm-linux-gnueabi-ld: drivers/net/ethernet/micrel/ks8851_common.o: in function
`ks8851_probe_common':
ks8851_common.c:(.text+0xf80): undefined reference to `__this_module'
Change the ks8851_common code to be a standalone module instead,
and use Makefile logic to ensure this is built-in if at least one
of its two users is.
Fixes: 797047f875b5 ("net: ks8851: Implement Parallel bus operations")
Signed-off-by: Arnd Bergmann
---
Marek sent two other patches to address the problem:
https://lore.kernel.org/netdev/20210116164828.40545-1-ma...@denx.de/
https://lore.kernel.org/netdev/20210115134239.126152-1-ma...@denx.de/
My version is what I applied locally to my randconfig tree, and
I think this is the cleanest solution.
---
drivers/net/ethernet/micrel/Makefile| 6 ++
drivers/net/ethernet/micrel/ks8851_common.c | 8
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/micrel/Makefile
b/drivers/net/ethernet/micrel/Makefile
index 5cc00d22c708..6ecc4eb30e74 100644
--- a/drivers/net/ethernet/micrel/Makefile
+++ b/drivers/net/ethernet/micrel/Makefile
@@ -4,8 +4,6 @@
#
obj-$(CONFIG_KS8842) += ks8842.o
-obj-$(CONFIG_KS8851) += ks8851.o
-ks8851-objs = ks8851_common.o ks8851_spi.o
-obj-$(CONFIG_KS8851_MLL) += ks8851_mll.o
-ks8851_mll-objs = ks8851_common.o ks8851_par.o
+obj-$(CONFIG_KS8851) += ks8851_common.o ks8851_spi.o
+obj-$(CONFIG_KS8851_MLL) += ks8851_common.o ks8851_par.o
obj-$(CONFIG_KSZ884X_PCI) += ksz884x.o
diff --git a/drivers/net/ethernet/micrel/ks8851_common.c
b/drivers/net/ethernet/micrel/ks8851_common.c
index 2feed6ce19d3..13d0623c88c6 100644
--- a/drivers/net/ethernet/micrel/ks8851_common.c
+++ b/drivers/net/ethernet/micrel/ks8851_common.c
@@ -1065,6 +1065,7 @@ int ks8851_suspend(struct device *dev)
return 0;
}
+EXPORT_SYMBOL_GPL(ks8851_suspend);
int ks8851_resume(struct device *dev)
{
@@ -1078,6 +1079,7 @@ int ks8851_resume(struct device *dev)
return 0;
}
+EXPORT_SYMBOL_GPL(ks8851_resume);
#endif
static int ks8851_register_mdiobus(struct ks8851_net *ks, struct device *dev)
@@ -1251,6 +1253,7 @@ int ks8851_probe_common(struct net_device *netdev, struct
device *dev,
err_reg_io:
return ret;
}
+EXPORT_SYMBOL_GPL(ks8851_probe_common);
int ks8851_remove_common(struct device *dev)
{
@@ -1269,3 +1272,8 @@ int ks8851_remove_common(struct device *dev)
return 0;
}
+EXPORT_SYMBOL_GPL(ks8851_remove_common);
+
+MODULE_DESCRIPTION("KS8851 Network driver");
+MODULE_AUTHOR("Ben Dooks ");
+MODULE_LICENSE("GPL");
--
2.29.2
