* Re: [PATCH 000/141] Fix fall-through warnings for Clang
From: James Bottomley @ 2020-11-22 19:12 UTC (permalink / raw)
To: Joe Perches, Kees Cook, Jakub Kicinski
Cc: Gustavo A. R. Silva, linux-kernel, alsa-devel, amd-gfx, bridge,
ceph-devel, cluster-devel, coreteam, devel, dm-devel, drbd-dev,
dri-devel, GR-everest-linux-l2, GR-Linux-NIC-Dev, intel-gfx,
intel-wired-lan, keyrings, linux1394-devel, linux-acpi, linux-afs,
linux-arm-kernel, linux-arm-msm, linux-atm-general, linux-block,
linux-can, linux-cifs, linux-crypto, linux-decnet-user,
linux-ext4, linux-fbdev, linux-geode, linux-gpio, linux-hams,
linux-hwmon, linux-i3c, linux-ide, linux-iio, linux-input,
linux-integrity, linux-mediatek, linux-media, linux-mmc, linux-mm,
linux-mtd, linux-nfs, linux-rdma, linux-renesas-soc, linux-scsi,
linux-sctp, linux-security-module, linux-stm32, linux-usb,
linux-watchdog, linux-wireless, netdev, netfilter-devel, nouveau,
op-tee, oss-drivers, patches, rds-devel, reiserfs-devel,
samba-technical, selinux, target-devel, tipc-discussion,
usb-storage, virtualization, wcn36xx, x86, xen-devel,
linux-hardening, Nick Desaulniers, Nathan Chancellor,
Miguel Ojeda
In-Reply-To: <ca071decb87cc7e905411423c05a48f9fd2f58d7.camel@perches.com>
On Sun, 2020-11-22 at 10:25 -0800, Joe Perches wrote:
> On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote:
> > Please tell me our reward for all this effort isn't a single
> > missing error print.
>
> There were quite literally dozens of logical defects found
> by the fallthrough additions. Very few were logging only.
So can you give us the best examples (or indeed all of them if someone
is keeping score)? hopefully this isn't a US election situation ...
James
^ permalink raw reply
* Re: [PATCH 000/141] Fix fall-through warnings for Clang
From: Joe Perches @ 2020-11-22 19:22 UTC (permalink / raw)
To: James Bottomley, Kees Cook, Jakub Kicinski
Cc: Gustavo A. R. Silva, linux-kernel, alsa-devel, amd-gfx, bridge,
ceph-devel, cluster-devel, coreteam, devel, dm-devel, drbd-dev,
dri-devel, GR-everest-linux-l2, GR-Linux-NIC-Dev, intel-gfx,
intel-wired-lan, keyrings, linux1394-devel, linux-acpi, linux-afs,
linux-arm-kernel, linux-arm-msm, linux-atm-general, linux-block,
linux-can, linux-cifs, linux-crypto, linux-decnet-user,
linux-ext4, linux-fbdev, linux-geode, linux-gpio, linux-hams,
linux-hwmon, linux-i3c, linux-ide, linux-iio, linux-input,
linux-integrity, linux-mediatek, linux-media, linux-mmc, linux-mm,
linux-mtd, linux-nfs, linux-rdma, linux-renesas-soc, linux-scsi,
linux-sctp, linux-security-module, linux-stm32, linux-usb,
linux-watchdog, linux-wireless, netdev, netfilter-devel, nouveau,
op-tee, oss-drivers, patches, rds-devel, reiserfs-devel,
samba-technical, selinux, target-devel, tipc-discussion,
usb-storage, virtualization, wcn36xx, x86, xen-devel,
linux-hardening, Nick Desaulniers, Nathan Chancellor,
Miguel Ojeda
In-Reply-To: <0147972a72bc13f3629de8a32dee6f1f308994b5.camel@HansenPartnership.com>
On Sun, 2020-11-22 at 11:12 -0800, James Bottomley wrote:
> On Sun, 2020-11-22 at 10:25 -0800, Joe Perches wrote:
> > On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote:
> > > Please tell me our reward for all this effort isn't a single
> > > missing error print.
> >
> > There were quite literally dozens of logical defects found
> > by the fallthrough additions. Very few were logging only.
>
> So can you give us the best examples (or indeed all of them if someone
> is keeping score)? hopefully this isn't a US election situation ...
Gustavo? Are you running for congress now?
https://lwn.net/Articles/794944/
^ permalink raw reply
* Re: [PATCH net-next 2/3] mlxsw: spectrum_ptp: use PTP wide message type definitions
From: Christian Eggers @ 2020-11-22 19:29 UTC (permalink / raw)
To: Ido Schimmel
Cc: Richard Cochran, Andrew Lunn, Heiner Kallweit, Jakub Kicinski,
Vladimir Oltean, Russell King, David S . Miller, netdev,
linux-kernel, Petr Machata, Jiri Pirko, Ido Schimmel
In-Reply-To: <20201122143555.GA515025@shredder.lan>
On Sunday, 22 November 2020, 15:35:55 CET, Ido Schimmel wrote:
> On Sun, Nov 22, 2020 at 09:26:35AM +0100, Christian Eggers wrote:
> > Use recently introduced PTP wide defines instead of a driver internal
> > enumeration.
> >
> > Signed-off-by: Christian Eggers <ceggers@gmx.de>
> > Cc: Petr Machata <petrm@mellanox.com>
> > Cc: Jiri Pirko <jiri@nvidia.com>
> > Cc: Ido Schimmel <idosch@nvidia.com>
>
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
>
> But:
>
> 1. Checkpatch complains about:
> WARNING: From:/Signed-off-by: email address mismatch: 'From: Christian
> Eggers <ceggers@arri.de>' != 'Signed-off-by: Christian Eggers
> <ceggers@gmx.de>'
unfortunately I changed this after running checkpatch. My intention was to
separate my (private) weekend work from the patches I do while I'm on the job.
> 2. This series does not build, which fails the CI [1][2] and also
> required me to fetch the dependencies that are currently under review
> [3]. I believe it is generally discouraged to create dependencies
> between patch sets that are under review for exactly these reasons.
this was also not by intention. Vladimir found some files I missed in the
first series. As the whole first series had already been reviewed at that time,
I wasn't sure whether I am allowed to add further patches to it. Additionally
I didn't concern that although my local build is successful, I should wait
until the first series is applied...
> I don't know what are Jakub's preferences, but had this happened on our
> internal patchwork instance, I would just ask the author to submit
> another version with all the patches.
Please let me know how I shall proceed...
> Anyway, I added all six patches to our regression as we have some PTP
> tests. Will let you know tomorrow.
>
> Thanks
>
> [1]
> https://lore.kernel.org/netdev/20201122082636.12451-1-ceggers@arri.de/T/#mc
> ef35858585d23b72b8f75450a51618d5c5d3260 [2]
> https://patchwork.hopto.org/static/nipa/389053/11923809/build_allmodconfig_
> warn/summary [3]
> https://patchwork.kernel.org/project/netdevbpf/cover/20201120084106.10046-1
> -ceggers@arri.de/
^ permalink raw reply
* Re: [PATCH] libbpf: add support for canceling cached_cons advance
From: kernel test robot @ 2020-11-22 19:42 UTC (permalink / raw)
To: Li RongQing, netdev, bpf; +Cc: kbuild-all
In-Reply-To: <1606050623-22963-1-git-send-email-lirongqing@baidu.com>
[-- Attachment #1: Type: text/plain, Size: 1858 bytes --]
Hi Li,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
[also build test ERROR on bpf/master ipvs/master v5.10-rc4 next-20201120]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Li-RongQing/libbpf-add-support-for-canceling-cached_cons-advance/20201122-212415
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-randconfig-a006-20201122 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/394b6e92389887aaf0ef8a2a70ef295b045c748b
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Li-RongQing/libbpf-add-support-for-canceling-cached_cons-advance/20201122-212415
git checkout 394b6e92389887aaf0ef8a2a70ef295b045c748b
# save the attached .config to linux build tree
make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from xsk.c:35:
xsk.h: In function 'xsk_ring_cons__cancel':
>> xsk.h:159:2: error: 'rx' undeclared (first use in this function)
159 | rx->cached_cons -= nb;
| ^~
xsk.h:159:2: note: each undeclared identifier is reported only once for each function it appears in
make[6]: *** [tools/build/Makefile.build:97: kernel/bpf/preload/staticobjs/xsk.o] Error 1
make[6]: Target '__build' not remade because of errors.
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 42936 bytes --]
^ permalink raw reply
* Re: [PATCH iproute2-next 0/2] ip: Nexthop flags extensions
From: David Ahern @ 2020-11-22 19:47 UTC (permalink / raw)
To: Ido Schimmel, netdev; +Cc: roopa, mlxsw, Ido Schimmel
In-Reply-To: <20201119135731.410986-1-idosch@idosch.org>
On 11/19/20 6:57 AM, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
>
> Patch #1 prints the recently added 'RTNH_F_TRAP' flag.
>
> Patch #2 makes sure that nexthop flags are always printed for nexthop
> objects. Even when the nexthop does not have a device, such as a
> blackhole nexthop or a group.
>
> Example output with netdevsim:
>
> # ip nexthop
> id 1 via 192.0.2.2 dev eth0 scope link trap
> id 2 blackhole trap
> id 3 group 2 trap
>
> Example output with mlxsw:
>
> # ip nexthop
> id 1 via 192.0.2.2 dev swp3 scope link offload
> id 2 blackhole offload
> id 3 group 2 offload
>
> Tested with fib_nexthops.sh that uses "ip nexthop" output:
>
> Tests passed: 164
> Tests failed: 0
>
> Ido Schimmel (2):
> ip route: Print "trap" nexthop indication
> nexthop: Always print nexthop flags
>
> ip/ipnexthop.c | 3 +--
> ip/iproute.c | 2 ++
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
applied to iproute2-next, without preferential treatment.
^ permalink raw reply
* Re: [PATCH 000/141] Fix fall-through warnings for Clang
From: James Bottomley @ 2020-11-22 19:53 UTC (permalink / raw)
To: Joe Perches, Kees Cook, Jakub Kicinski
Cc: Gustavo A. R. Silva, linux-kernel, alsa-devel, amd-gfx, bridge,
ceph-devel, cluster-devel, coreteam, devel, dm-devel, drbd-dev,
dri-devel, GR-everest-linux-l2, GR-Linux-NIC-Dev, intel-gfx,
intel-wired-lan, keyrings, linux1394-devel, linux-acpi, linux-afs,
linux-arm-kernel, linux-arm-msm, linux-atm-general, linux-block,
linux-can, linux-cifs, linux-crypto, linux-decnet-user,
linux-ext4, linux-fbdev, linux-geode, linux-gpio, linux-hams,
linux-hwmon, linux-i3c, linux-ide, linux-iio, linux-input,
linux-integrity, linux-mediatek, linux-media, linux-mmc, linux-mm,
linux-mtd, linux-nfs, linux-rdma, linux-renesas-soc, linux-scsi,
linux-sctp, linux-security-module, linux-stm32, linux-usb,
linux-watchdog, linux-wireless, netdev, netfilter-devel, nouveau,
op-tee, oss-drivers, patches, rds-devel, reiserfs-devel,
samba-technical, selinux, target-devel, tipc-discussion,
usb-storage, virtualization, wcn36xx, x86, xen-devel,
linux-hardening, Nick Desaulniers, Nathan Chancellor,
Miguel Ojeda
In-Reply-To: <d8d1e9add08cdd4158405e77762d4946037208f8.camel@perches.com>
On Sun, 2020-11-22 at 11:22 -0800, Joe Perches wrote:
> On Sun, 2020-11-22 at 11:12 -0800, James Bottomley wrote:
> > On Sun, 2020-11-22 at 10:25 -0800, Joe Perches wrote:
> > > On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote:
> > > > Please tell me our reward for all this effort isn't a single
> > > > missing error print.
> > >
> > > There were quite literally dozens of logical defects found
> > > by the fallthrough additions. Very few were logging only.
> >
> > So can you give us the best examples (or indeed all of them if
> > someone is keeping score)? hopefully this isn't a US election
> > situation ...
>
> Gustavo? Are you running for congress now?
>
> https://lwn.net/Articles/794944/
That's 21 reported fixes of which about 50% seem to produce no change
in code behaviour at all, a quarter seem to have no user visible effect
with the remaining quarter producing unexpected errors on obscure
configuration parameters, which is why no-one really noticed them
before.
James
^ permalink raw reply
* Re: [PATCH net-next 2/3] mlxsw: spectrum_ptp: use PTP wide message type definitions
From: Ido Schimmel @ 2020-11-22 20:01 UTC (permalink / raw)
To: Christian Eggers
Cc: Richard Cochran, Andrew Lunn, Heiner Kallweit, Jakub Kicinski,
Vladimir Oltean, Russell King, David S . Miller, netdev,
linux-kernel, Petr Machata, Jiri Pirko, Ido Schimmel
In-Reply-To: <2074851.ybSLjXPktx@n95hx1g2>
On Sun, Nov 22, 2020 at 08:29:22PM +0100, Christian Eggers wrote:
> On Sunday, 22 November 2020, 15:35:55 CET, Ido Schimmel wrote:
> > On Sun, Nov 22, 2020 at 09:26:35AM +0100, Christian Eggers wrote:
> > > Use recently introduced PTP wide defines instead of a driver internal
> > > enumeration.
> > >
> > > Signed-off-by: Christian Eggers <ceggers@gmx.de>
> > > Cc: Petr Machata <petrm@mellanox.com>
> > > Cc: Jiri Pirko <jiri@nvidia.com>
> > > Cc: Ido Schimmel <idosch@nvidia.com>
> >
> > Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> >
> > But:
> >
> > 1. Checkpatch complains about:
> > WARNING: From:/Signed-off-by: email address mismatch: 'From: Christian
> > Eggers <ceggers@arri.de>' != 'Signed-off-by: Christian Eggers
> > <ceggers@gmx.de>'
> unfortunately I changed this after running checkpatch. My intention was to
> separate my (private) weekend work from the patches I do while I'm on the job.
No problem. Just make sure that authorship and Signed-off-by agree. You
can use:
# git commit --amend --author="Christian Eggers <ceggers@gmx.de>"
>
> > 2. This series does not build, which fails the CI [1][2] and also
> > required me to fetch the dependencies that are currently under review
> > [3]. I believe it is generally discouraged to create dependencies
> > between patch sets that are under review for exactly these reasons.
> this was also not by intention. Vladimir found some files I missed in the
> first series. As the whole first series had already been reviewed at that time,
> I wasn't sure whether I am allowed to add further patches to it. Additionally
> I didn't concern that although my local build is successful, I should wait
> until the first series is applied...
Yea, I saw that, no problem :)
>
> > I don't know what are Jakub's preferences, but had this happened on our
> > internal patchwork instance, I would just ask the author to submit
> > another version with all the patches.
> Please let me know how I shall proceed...
Jakub has the final say, so I assume he will comment on that.
Regardless, thanks for the patches.
^ permalink raw reply
* Re: [PATCH net-next,v5 0/9] netfilter: flowtable bridge and vlan enhancements
From: Pablo Neira Ayuso @ 2020-11-22 20:15 UTC (permalink / raw)
To: Alexander Lobakin
Cc: netfilter-devel, davem, netdev, kuba, fw, razor, jeremy, tobias,
linux-kernel
In-Reply-To: <20201122145108.2640-1-alobakin@pm.me>
On Sun, Nov 22, 2020 at 02:51:18PM +0000, Alexander Lobakin wrote:
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Date: Sun, 22 Nov 2020 12:42:19 +0100
>
> > On Sun, Nov 22, 2020 at 10:26:16AM +0000, Alexander Lobakin wrote:
> >> From: Pablo Neira Ayuso <pablo@netfilter.org>
> >> Date: Fri, 20 Nov 2020 13:49:12 +0100
> > [...]
> >>> Something like this:
> >>>
> >>> fast path
> >>> .------------------------.
> >>> / \
> >>> | IP forwarding |
> >>> | / \ .
> >>> | br0 eth0
> >>> . / \
> >>> -- veth1 veth2
> >>> .
> >>> .
> >>> .
> >>> eth0
> >>> ab:cd:ef:ab:cd:ef
> >>> VM
> >>
> >> I'm concerned about bypassing vlan and bridge's .ndo_start_xmit() in
> >> case of this shortcut. We'll have incomplete netdevice Tx stats for
> >> these two, as it gets updated inside this callbacks.
> >
> > TX device stats are being updated accordingly.
> >
> > # ip netns exec nsr1 ip -s link
> > 1: lo: <LOOPBACK> mtu 65536 qdisc noop state DOWN mode DEFAULT group default qlen 1000
> > link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
> > RX: bytes packets errors dropped overrun mcast
> > 0 0 0 0 0 0
> > TX: bytes packets errors dropped carrier collsns
> > 0 0 0 0 0 0
> > 2: veth0@if2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
> > link/ether 82:0d:f3:b5:59:5d brd ff:ff:ff:ff:ff:ff link-netns ns1
> > RX: bytes packets errors dropped overrun mcast
> > 213290848248 4869765 0 0 0 0
> > TX: bytes packets errors dropped carrier collsns
> > 315346667 4777953 0 0 0 0
> > 3: veth1@if2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
> > link/ether 4a:81:2d:9a:02:88 brd ff:ff:ff:ff:ff:ff link-netns ns2
> > RX: bytes packets errors dropped overrun mcast
> > 315337919 4777833 0 0 0 0
> > TX: bytes packets errors dropped carrier collsns
> > 213290844826 4869708 0 0 0 0
> > 4: br0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
> > link/ether 82:0d:f3:b5:59:5d brd ff:ff:ff:ff:ff:ff
> > RX: bytes packets errors dropped overrun mcast
> > 4101 73 0 0 0 0
> > TX: bytes packets errors dropped carrier collsns
> > 5256 74 0 0 0 0
>
> Aren't these counters very low for br0, despite that br0 is an
> intermediate point of traffic flow?
Most packets follow the flowtable fast path, which is bypassing the
br0 device. Bumping br0 stats would be misleading, it would make the
user think that the packets follow the classic bridge layer path,
while they do not. The flowtable have counters itself to allow the
user to collect stats regarding the packets that follow the fastpath.
^ permalink raw reply
* Re: [PATCH 000/141] Fix fall-through warnings for Clang
From: Miguel Ojeda @ 2020-11-22 20:35 UTC (permalink / raw)
To: James Bottomley
Cc: Kees Cook, Jakub Kicinski, Gustavo A. R. Silva, linux-kernel,
alsa-devel, amd-gfx, bridge, ceph-devel, cluster-devel, coreteam,
devel, dm-devel, drbd-dev, dri-devel, GR-everest-linux-l2,
GR-Linux-NIC-Dev, intel-gfx, intel-wired-lan, keyrings,
linux1394-devel, linux-acpi, linux-afs, Linux ARM, linux-arm-msm,
linux-atm-general, linux-block, linux-can, linux-cifs,
Linux Crypto Mailing List, linux-decnet-user,
Ext4 Developers List, linux-fbdev, linux-geode, linux-gpio,
linux-hams, linux-hwmon, linux-i3c, linux-ide, linux-iio,
linux-input, linux-integrity, linux-mediatek,
Linux Media Mailing List, linux-mmc, Linux-MM, linux-mtd,
linux-nfs, linux-rdma, linux-renesas-soc, linux-scsi, linux-sctp,
linux-security-module, linux-stm32, linux-usb, linux-watchdog,
linux-wireless, Network Development, netfilter-devel, nouveau,
op-tee, oss-drivers, patches, rds-devel, reiserfs-devel,
samba-technical, selinux, target-devel, tipc-discussion,
usb-storage, virtualization, wcn36xx,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), xen-devel,
linux-hardening, Nick Desaulniers, Nathan Chancellor,
Miguel Ojeda, Joe Perches
In-Reply-To: <9b57fd4914b46f38d54087d75e072d6e947cb56d.camel@HansenPartnership.com>
On Sun, Nov 22, 2020 at 7:22 PM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> Well, it's a problem in an error leg, sure, but it's not a really
> compelling reason for a 141 patch series, is it? All that fixing this
> error will do is get the driver to print "oh dear there's a problem"
> under four more conditions than it previously did.
>
> We've been at this for three years now with nearly a thousand patches,
> firstly marking all the fall throughs with /* fall through */ and later
> changing it to fallthrough. At some point we do have to ask if the
> effort is commensurate with the protection afforded. Please tell me
> our reward for all this effort isn't a single missing error print.
It isn't that much effort, isn't it? Plus we need to take into account
the future mistakes that it might prevent, too. So even if there were
zero problems found so far, it is still a positive change.
I would agree if these changes were high risk, though; but they are
almost trivial.
Cheers,
Miguel
^ permalink raw reply
* [PATCH] net: mlx5e: fix fs_tcp.c build when IPV6 is not enabled
From: Randy Dunlap @ 2020-11-22 21:12 UTC (permalink / raw)
To: netdev
Cc: Randy Dunlap, kernel test robot, Saeed Mahameed, Boris Pismenny,
Tariq Toukan, David S. Miller, Jakub Kicinski
Fix build when CONFIG_IPV6 is not enabled by making a function
be built conditionally.
Fixes these build errors and warnings:
../drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c: In function 'accel_fs_tcp_set_ipv6_flow':
../include/net/sock.h:380:34: error: 'struct sock_common' has no member named 'skc_v6_daddr'; did you mean 'skc_daddr'?
380 | #define sk_v6_daddr __sk_common.skc_v6_daddr
| ^~~~~~~~~~~~
../drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c:55:14: note: in expansion of macro 'sk_v6_daddr'
55 | &sk->sk_v6_daddr, 16);
| ^~~~~~~~~~~
At top level:
../drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c:47:13: warning: 'accel_fs_tcp_set_ipv6_flow' defined but not used [-Wunused-function]
47 | static void accel_fs_tcp_set_ipv6_flow(struct mlx5_flow_spec *spec, struct sock *sk)
Fixes: 5229a96e59ec ("net/mlx5e: Accel, Expose flow steering API for rules add/del")
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Reported-by: kernel test robot <lkp@intel.com>
Cc: Saeed Mahameed <saeedm@nvidia.com>
Cc: Boris Pismenny <borisp@nvidia.com>
Cc: Tariq Toukan <tariqt@mellanox.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
---
drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c | 2 ++
1 file changed, 2 insertions(+)
--- linux-next-20201120.orig/drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c
+++ linux-next-20201120/drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c
@@ -44,6 +44,7 @@ static void accel_fs_tcp_set_ipv4_flow(s
outer_headers.dst_ipv4_dst_ipv6.ipv4_layout.ipv4);
}
+#if IS_ENABLED(CONFIG_IPV6)
static void accel_fs_tcp_set_ipv6_flow(struct mlx5_flow_spec *spec, struct sock *sk)
{
MLX5_SET_TO_ONES(fte_match_param, spec->match_criteria, outer_headers.ip_protocol);
@@ -63,6 +64,7 @@ static void accel_fs_tcp_set_ipv6_flow(s
outer_headers.dst_ipv4_dst_ipv6.ipv6_layout.ipv6),
0xff, 16);
}
+#endif
void mlx5e_accel_fs_del_sk(struct mlx5_flow_handle *rule)
{
^ permalink raw reply
* Re: [PATCH v4] aquantia: Remove the build_skb path
From: Ramsay, Lincoln @ 2020-11-22 21:55 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Florian Westphal, Igor Russkikh, David S. Miller,
netdev@vger.kernel.org, Dmitry Bogdanov
In-Reply-To: <20201121132204.43f9c4fb@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
> Align continuations of the lines under '(' like:
Oh... I didn't run the patch checker over this revised patch. In this case, I am only changing the leading indent. Am I still expected to satisfy the patch checker?
The current patch is very clear about what is happening if you do a diff -w but if I start changing other things to satisfy the checker, that goes away.
I can do that... just double checking that it's wanted...
Lincoln
^ permalink raw reply
* Re: [PATCH net-next 2/3] mlxsw: spectrum_ptp: use PTP wide message type definitions
From: Vladimir Oltean @ 2020-11-22 22:01 UTC (permalink / raw)
To: Christian Eggers
Cc: Ido Schimmel, Richard Cochran, Andrew Lunn, Heiner Kallweit,
Jakub Kicinski, Russell King, David S . Miller, netdev,
linux-kernel, Petr Machata, Jiri Pirko, Ido Schimmel
In-Reply-To: <2074851.ybSLjXPktx@n95hx1g2>
On Sun, Nov 22, 2020 at 08:29:22PM +0100, Christian Eggers wrote:
> this was also not by intention. Vladimir found some files I missed in the
> first series. As the whole first series had already been reviewed at that time,
> I wasn't sure whether I am allowed to add further patches to it. Additionally
> I didn't concern that although my local build is successful, I should wait
> until the first series is applied...
When I said that, what I was thinking of (although it might have not
been clear) was that if you send further patches, you send them _after_
the initial series is merged.
Alternatively, it would have been just as valid to resend the entire
series, as a 3+3 patchset with a new revision (v3 -> v4), with review
tags collected from the first three, and the last 3 being completely
new. Jakub could have superseded v3 and applied v4.
The idea behind splicing N patches into a series is that they are
logically connected to one another. For example, a patch doesn't build
without another. You _could_ split logically-connected patches into
separate series and post them independently for review, as long as they
are build-time independent. If they aren't, well, what happens is
exactly what happened: various test robots will report build failures,
which from a maintainer's point of view inspires less confidence to
apply a patch as-is. I would not be surprised if Jakub asked you to
resend with no change at all, just to ensure that the patch series
passes build tests before merging it.
^ permalink raw reply
* Re: [PATCH 000/141] Fix fall-through warnings for Clang
From: Sam Ravnborg @ 2020-11-22 22:10 UTC (permalink / raw)
To: James Bottomley
Cc: Kees Cook, Jakub Kicinski, alsa-devel, linux-atm-general,
reiserfs-devel, linux-iio, linux-wireless, linux-fbdev, dri-devel,
linux-kernel, Nathan Chancellor, linux-ide, dm-devel, keyrings,
linux-mtd, GR-everest-linux-l2, wcn36xx, samba-technical,
linux-i3c, linux1394-devel, linux-afs, usb-storage, drbd-dev,
devel, linux-cifs, rds-devel, Nick Desaulniers, linux-scsi,
linux-rdma, oss-drivers, bridge, linux-security-module, amd-gfx,
linux-stm32, cluster-devel, linux-acpi, coreteam, intel-wired-lan,
linux-input, Miguel Ojeda, tipc-discussion, linux-ext4,
linux-media, linux-watchdog, selinux, linux-arm-msm, intel-gfx,
linux-geode, linux-can, linux-block, linux-gpio, op-tee,
linux-mediatek, xen-devel, nouveau, linux-hams, ceph-devel,
virtualization, target-devel, linux-arm-kernel, linux-hwmon, x86,
linux-nfs, GR-Linux-NIC-Dev, linux-mm, netdev, linux-decnet-user,
linux-mmc, Gustavo A. R. Silva, linux-renesas-soc, linux-sctp,
linux-usb, netfilter-devel, linux-crypto, patches, Joe Perches,
linux-integrity, linux-hardening
In-Reply-To: <9b57fd4914b46f38d54087d75e072d6e947cb56d.camel@HansenPartnership.com>
Hi James.
> > > If none of the 140 patches here fix a real bug, and there is no
> > > change to machine code then it sounds to me like a W=2 kind of a
> > > warning.
> >
> > FWIW, this series has found at least one bug so far:
> > https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=Kr_CQ@mail.gmail.com/
>
>
> Well, it's a problem in an error leg, sure, but it's not a really
> compelling reason for a 141 patch series, is it? All that fixing this
> error will do is get the driver to print "oh dear there's a problem"
> under four more conditions than it previously did.
You are asking the wrong question here.
Yuo should ask how many hours could have been saved by all the bugs
people have been fighting with and then fixed *before* the code
hit the kernel at all.
My personal experience is that I, more than once, have had errors
related to a missing break in my code. So this warnings is IMO a win.
And if we are only ~100 patches to have it globally enabled then it is a
no-brainer in my book.
Sam
^ permalink raw reply
* Re: [RFC] MAINTAINERS tag for cleanup robot
From: Finn Thain @ 2020-11-22 22:33 UTC (permalink / raw)
To: Joe Perches
Cc: James Bottomley, Tom Rix, Matthew Wilcox, clang-built-linux,
linux-hyperv, linux-kernel, xen-devel, tboot-devel, kvm,
linux-crypto, linux-acpi, devel, amd-gfx, dri-devel, intel-gfx,
netdev, linux-media, MPT-FusionLinux.pdl, linux-scsi,
linux-wireless, ibm-acpi-devel, platform-driver-x86, linux-usb,
linux-omap, linux-fbdev, ecryptfs, linux-fsdevel, cluster-devel,
linux-mtd, keyrings, netfilter-devel, coreteam, alsa-devel, bpf,
linux-bluetooth, linux-nfs, patches
In-Reply-To: <dec07021e7fc11a02b14c98b713ae2c6e2a4ca00.camel@perches.com>
On Sun, 22 Nov 2020, Joe Perches wrote:
> On Sun, 2020-11-22 at 08:49 -0800, James Bottomley wrote:
> > We can enforce sysfs_emit going forwards
> > using tools like checkpatch
>
> It's not really possible for checkpatch to find or warn about
> sysfs uses of sprintf. checkpatch is really just a trivial
> line-by-line parser and it has no concept of code intent.
>
Checkpatch does suffer from the limitations of regular expressions. But
that doesn't stop people from using it. Besides, Coccinelle can do
analyses that can't be done with regular expressions, so it's moot.
> It just can't warn on every use of the sprintf family.
> There are just too many perfectly valid uses.
>
> > but there's no benefit and a lot of harm to
> > be done by trying to churn the entire tree
>
> Single uses of sprintf for sysfs is not really any problem.
>
> But likely there are still several possible overrun sprintf/snprintf
> paths in sysfs. Some of them are very obscure and unlikely to be
> found by a robot as the logic for sysfs buf uses can be fairly twisty.
>
Logic errors of this kind are susceptible to fuzzing, formal methods,
symbolic execution etc. No doubt there are other techniques that I don't
know about.
> But provably correct conversions IMO _should_ be done and IMO churn
> considerations should generally have less importance.
>
Provably equivalent conversions are provably churn. So apparently you're
advocating changes that are not provably equivalent.
These are patches for code not that's not been shown to be buggy. Code
which, after patching, can be shown to be free of a specific kind of
theoretical bug. Hardly "provably correct".
The problem is, the class of theoretical bugs that can be avoided in this
way is probably limitless, as is the review cost and the risk of
accidental regressions. And the payoff is entirely theoretical.
Moreover, the patch review workload for skilled humans is being generated
by the automation, which is completely backwards: the machine is supposed
to be helping.
^ permalink raw reply
* Re: [PATCH 000/141] Fix fall-through warnings for Clang
From: James Bottomley @ 2020-11-22 22:36 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Kees Cook, Jakub Kicinski, Gustavo A. R. Silva, linux-kernel,
alsa-devel, amd-gfx, bridge, ceph-devel, cluster-devel, coreteam,
devel, dm-devel, drbd-dev, dri-devel, GR-everest-linux-l2,
GR-Linux-NIC-Dev, intel-gfx, intel-wired-lan, keyrings,
linux1394-devel, linux-acpi, linux-afs, Linux ARM, linux-arm-msm,
linux-atm-general, linux-block, linux-can, linux-cifs,
Linux Crypto Mailing List, linux-decnet-user,
Ext4 Developers List, linux-fbdev, linux-geode, linux-gpio,
linux-hams, linux-hwmon, linux-i3c, linux-ide, linux-iio,
linux-input, linux-integrity, linux-mediatek,
Linux Media Mailing List, linux-mmc, Linux-MM, linux-mtd,
linux-nfs, linux-rdma, linux-renesas-soc, linux-scsi, linux-sctp,
linux-security-module, linux-stm32, linux-usb, linux-watchdog,
linux-wireless, Network Development, netfilter-devel, nouveau,
op-tee, oss-drivers, patches, rds-devel, reiserfs-devel,
samba-technical, selinux, target-devel, tipc-discussion,
usb-storage, virtualization, wcn36xx,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), xen-devel,
linux-hardening, Nick Desaulniers, Nathan Chancellor,
Miguel Ojeda, Joe Perches
In-Reply-To: <CANiq72nZrHWTA4_Msg6MP9snTyenC6-eGfD27CyfNSu7QoVZbw@mail.gmail.com>
On Sun, 2020-11-22 at 21:35 +0100, Miguel Ojeda wrote:
> On Sun, Nov 22, 2020 at 7:22 PM James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > Well, it's a problem in an error leg, sure, but it's not a really
> > compelling reason for a 141 patch series, is it? All that fixing
> > this error will do is get the driver to print "oh dear there's a
> > problem" under four more conditions than it previously did.
> >
> > We've been at this for three years now with nearly a thousand
> > patches, firstly marking all the fall throughs with /* fall through
> > */ and later changing it to fallthrough. At some point we do have
> > to ask if the effort is commensurate with the protection
> > afforded. Please tell me our reward for all this effort isn't a
> > single missing error print.
>
> It isn't that much effort, isn't it?
Well, it seems to be three years of someone's time plus the maintainer
review time and series disruption of nearly a thousand patches. Let's
be conservative and assume the producer worked about 30% on the series
and it takes about 5-10 minutes per patch to review, merge and for
others to rework existing series. So let's say it's cost a person year
of a relatively junior engineer producing the patches and say 100h of
review and application time. The latter is likely the big ticket item
because it's what we have in least supply in the kernel (even though
it's 20x vs the producer time).
> Plus we need to take into account the future mistakes that it might
> prevent, too. So even if there were zero problems found so far, it is
> still a positive change.
Well, the question I was asking is if it's worth the cost which I've
tried to outline above.
> I would agree if these changes were high risk, though; but they are
> almost trivial.
It's not about the risk of the changes it's about the cost of
implementing them. Even if you discount the producer time (which
someone gets to pay for, and if I were the engineering manager, I'd be
unhappy about), the review/merge/rework time is pretty significant in
exchange for six minor bug fixes. Fine, when a new compiler warning
comes along it's certainly reasonable to see if we can benefit from it
and the fact that the compiler people think it's worthwhile is enough
evidence to assume this initially. But at some point you have to ask
whether that assumption is supported by the evidence we've accumulated
over the time we've been using it. And if the evidence doesn't support
it perhaps it is time to stop the experiment.
James
^ permalink raw reply
* Re: [PATCH v4] aquantia: Remove the build_skb path
From: Ramsay, Lincoln @ 2020-11-22 22:36 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Florian Westphal, Igor Russkikh, David S. Miller,
netdev@vger.kernel.org, Dmitry Bogdanov
In-Reply-To: <20201121132324.72d79e94@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
> (Next time please include in the subject the tree that you're targetting
> the patch)
I guess you mean like [PATCH master v5] ? Should I be targeting something other than the master branch on the main git repo? (https://github.com/torvalds/linux.git)
> please add a From: line at the beginning of the mail which matches
> the signoff (or use git-send-email, it'll get it right).
Sure.
> Ah, one more thing, this is the correct fixes tag, right?
> Fixes: 018423e90bee ("net: ethernet: aquantia: Add ring support code")
> Please add it right before the signoff line.
I didn't quite understand this header... but yeah, I guess that's the commit that adds the fast path I am removing.
> > Align continuations of the lines under '(' like:
>
> I am only changing the leading indent. Am I still expected to satisfy the patch checker?
>
> The current patch is very clear about what is happening if you do a diff -w but if I start
> changing other things to satisfy the checker, that goes away.
Some of the patch checker complaints are only leading whitespace (obviously not a problem for diff -w), but 2 of them involve actual changes (changing , to ; and moving the first argument from the line below to the line above).
Lincoln
^ permalink raw reply
* Re: [PATCH 000/141] Fix fall-through warnings for Clang
From: Finn Thain @ 2020-11-22 22:54 UTC (permalink / raw)
To: Miguel Ojeda
Cc: James Bottomley, Kees Cook, Jakub Kicinski, Gustavo A. R. Silva,
linux-kernel, alsa-devel, amd-gfx, bridge, ceph-devel,
cluster-devel, coreteam, devel, dm-devel, drbd-dev, dri-devel,
GR-everest-linux-l2, GR-Linux-NIC-Dev, intel-gfx, intel-wired-lan,
keyrings, linux1394-devel, linux-acpi, linux-afs, Linux ARM,
linux-arm-msm, linux-atm-general, linux-block, linux-can,
linux-cifs, Linux Crypto Mailing List, linux-decnet-user,
Ext4 Developers List, linux-fbdev, linux-geode, linux-gpio,
linux-hams, linux-hwmon, linux-i3c, linux-ide, linux-iio,
linux-input, linux-integrity, linux-mediatek,
Linux Media Mailing List, linux-mmc, Linux-MM, linux-mtd,
linux-nfs, linux-rdma, linux-renesas-soc, linux-scsi, linux-sctp,
linux-security-module, linux-stm32, linux-usb, linux-watchdog,
linux-wireless, Network Development, netfilter-devel, nouveau,
op-tee, oss-drivers, patches, rds-devel, reiserfs-devel,
samba-technical, selinux, target-devel, tipc-discussion,
usb-storage, virtualization, wcn36xx,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), xen-devel,
linux-hardening, Nick Desaulniers, Nathan Chancellor,
Miguel Ojeda, Joe Perches
In-Reply-To: <CANiq72nZrHWTA4_Msg6MP9snTyenC6-eGfD27CyfNSu7QoVZbw@mail.gmail.com>
On Sun, 22 Nov 2020, Miguel Ojeda wrote:
>
> It isn't that much effort, isn't it? Plus we need to take into account
> the future mistakes that it might prevent, too.
We should also take into account optimisim about future improvements in
tooling.
> So even if there were zero problems found so far, it is still a positive
> change.
>
It is if you want to spin it that way.
> I would agree if these changes were high risk, though; but they are
> almost trivial.
>
This is trivial:
case 1:
this();
+ fallthrough;
case 2:
that();
But what we inevitably get is changes like this:
case 3:
this();
+ break;
case 4:
hmmm();
Why? Mainly to silence the compiler. Also because the patch author argued
successfully that they had found a theoretical bug, often in mature code.
But is anyone keeping score of the regressions? If unreported bugs count,
what about unreported regressions?
> Cheers,
> Miguel
>
^ permalink raw reply
* Re: [PATCH 000/141] Fix fall-through warnings for Clang
From: James Bottomley @ 2020-11-22 23:04 UTC (permalink / raw)
To: Finn Thain, Miguel Ojeda
Cc: Kees Cook, Jakub Kicinski, Gustavo A. R. Silva, linux-kernel,
alsa-devel, amd-gfx, bridge, ceph-devel, cluster-devel, coreteam,
devel, dm-devel, drbd-dev, dri-devel, GR-everest-linux-l2,
GR-Linux-NIC-Dev, intel-gfx, intel-wired-lan, keyrings,
linux1394-devel, linux-acpi, linux-afs, Linux ARM, linux-arm-msm,
linux-atm-general, linux-block, linux-can, linux-cifs,
Linux Crypto Mailing List, linux-decnet-user,
Ext4 Developers List, linux-fbdev, linux-geode, linux-gpio,
linux-hams, linux-hwmon, linux-i3c, linux-ide, linux-iio,
linux-input, linux-integrity, linux-mediatek,
Linux Media Mailing List, linux-mmc, Linux-MM, linux-mtd,
linux-nfs, linux-rdma, linux-renesas-soc, linux-scsi, linux-sctp,
linux-security-module, linux-stm32, linux-usb, linux-watchdog,
linux-wireless, Network Development, netfilter-devel, nouveau,
op-tee, oss-drivers, patches, rds-devel, reiserfs-devel,
samba-technical, selinux, target-devel, tipc-discussion,
usb-storage, virtualization, wcn36xx,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), xen-devel,
linux-hardening, Nick Desaulniers, Nathan Chancellor,
Miguel Ojeda, Joe Perches
In-Reply-To: <alpine.LNX.2.23.453.2011230938390.7@nippy.intranet>
On Mon, 2020-11-23 at 09:54 +1100, Finn Thain wrote:
> But is anyone keeping score of the regressions? If unreported bugs
> count, what about unreported regressions?
Well, I was curious about the former (obviously no tool will tell me
about the latter), so I asked git what patches had a fall-through
series named in a fixes tag and these three popped out:
9cf51446e686 bpf, powerpc: Fix misuse of fallthrough in bpf_jit_comp()
6a9dc5fd6170 lib: Revert use of fallthrough pseudo-keyword in lib/
91dbd73a1739 mips/oprofile: Fix fallthrough placement
I don't think any of these is fixing a significant problem, but they
did cause someone time and trouble to investigate.
James
^ permalink raw reply
* Re: [PATCH net-next 1/1] net: dsa: hellcreek: Add TAPRIO offloading support
From: Vladimir Oltean @ 2020-11-22 23:08 UTC (permalink / raw)
To: Kurt Kanzenbach
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
Jakub Kicinski, Vinicius Costa Gomes, netdev
In-Reply-To: <20201121115703.23221-2-kurt@linutronix.de>
Hi Kurt,
On Sat, Nov 21, 2020 at 12:57:03PM +0100, Kurt Kanzenbach wrote:
> The switch has support for the 802.1Qbv Time Aware Shaper (TAS). Traffic
> schedules may be configured individually on each front port. Each port has eight
> egress queues. The traffic is mapped to a traffic class respectively via the PCP
> field of a VLAN tagged frame.
>
> The TAPRIO Qdisc already implements that. Therefore, this interface can simply
> be reused. Add .port_setup_tc() accordingly.
>
> The activation of a schedule on a port is split into two parts:
>
> * Programming the necessary gate control list (GCL)
> * Setup delayed work for starting the schedule
>
> The hardware supports starting a schedule up to eight seconds in the future. The
> TAPRIO interface provides an absolute base time. Therefore, periodic delayed
> work is leveraged to check whether a schedule may be started or not.
>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
> drivers/net/dsa/hirschmann/hellcreek.c | 314 +++++++++++++++++++++++++
> drivers/net/dsa/hirschmann/hellcreek.h | 22 ++
> 2 files changed, 336 insertions(+)
>
> diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
> index 6420b76ea37c..35514af1922a 100644
> --- a/drivers/net/dsa/hirschmann/hellcreek.c
> +++ b/drivers/net/dsa/hirschmann/hellcreek.c
> @@ -23,6 +23,7 @@
> #include <linux/mutex.h>
> #include <linux/delay.h>
> #include <net/dsa.h>
> +#include <net/pkt_sched.h>
>
> #include "hellcreek.h"
> #include "hellcreek_ptp.h"
> @@ -153,6 +154,13 @@ static void hellcreek_select_vlan(struct hellcreek *hellcreek, int vid,
> hellcreek_write(hellcreek, val, HR_VIDCFG);
> }
>
> +static void hellcreek_select_tgd(struct hellcreek *hellcreek, int port)
> +{
> + u16 val = port << TR_TGDSEL_TDGSEL_SHIFT;
> +
> + hellcreek_write(hellcreek, val, TR_TGDSEL);
> +}
> +
> static int hellcreek_wait_until_ready(struct hellcreek *hellcreek)
> {
> u16 val;
> @@ -1135,6 +1143,308 @@ hellcreek_port_prechangeupper(struct dsa_switch *ds, int port,
> return ret;
> }
>
> +static void hellcreek_setup_gcl(struct hellcreek *hellcreek, int port,
> + const struct hellcreek_schedule *schedule)
> +{
> + size_t i;
> +
> + for (i = 1; i <= schedule->num_entries; ++i) {
> + const struct hellcreek_gcl_entry *cur, *initial, *next;
> + u16 data;
> + u8 gates;
> +
> + cur = &schedule->entries[i - 1];
> + initial = &schedule->entries[0];
> + next = &schedule->entries[i];
> +
I would find it more intuitive to have the invariant assignment out of
the loop.
const struct hellcreek_gcl_entry *cur, *initial, *next;
cur = initial = &schedule->entries[0];
next = cur + 1;
for (i = 0; i < schedule->num_entries; i++) {
u16 data;
u8 gates;
[...]
cur++;
next++;
}
> + if (i == schedule->num_entries)
> + gates = initial->gate_states ^
> + cur->gate_states;
> + else
> + gates = next->gate_states ^
> + cur->gate_states;
> +
> + data = gates;
> + if (cur->overrun_ignore)
> + data |= TR_GCLDAT_GCLOVRI;
> +
> + if (i == schedule->num_entries)
> + data |= TR_GCLDAT_GCLWRLAST;
> +
> + /* Gates states */
> + hellcreek_write(hellcreek, data, TR_GCLDAT);
> +
> + /* Time intervall */
interval
> + hellcreek_write(hellcreek,
> + cur->interval & 0x0000ffff,
> + TR_GCLTIL);
> + hellcreek_write(hellcreek,
> + (cur->interval & 0xffff0000) >> 16,
> + TR_GCLTIH);
> +
> + /* Commit entry */
> + data = ((i - 1) << TR_GCLCMD_GCLWRADR_SHIFT) |
> + (initial->gate_states <<
> + TR_GCLCMD_INIT_GATE_STATES_SHIFT);
> + hellcreek_write(hellcreek, data, TR_GCLCMD);
So the command register holds the initial gate states. When the command
register is written, it samples the data register, populating GCL entry
number GCLWRADR with that information. Every GCL entry contains the
duration until it is executed, and the gates that need to change when
the schedule transitions towards the next GCL entry. Right?
Why are the initial gate states written each time into the command
register? Is it required by the hardware, or just easier in the
implementation?
> + }
> +}
> +
> +static void hellcreek_set_cycle_time(struct hellcreek *hellcreek,
> + const struct hellcreek_schedule *schedule)
> +{
> + u32 cycle_time = schedule->cycle_time;
> +
> + hellcreek_write(hellcreek, cycle_time & 0x0000ffff, TR_CTWRL);
> + hellcreek_write(hellcreek, (cycle_time & 0xffff0000) >> 16, TR_CTWRH);
The cycle_time in struct tc_taprio_qopt_offload is 64-bit, so you should
NACK a value that is too large and return an appropriate extack message.
> +}
> +
> +static void hellcreek_switch_schedule(struct hellcreek *hellcreek,
> + ktime_t start_time)
> +{
> + struct timespec64 ts = ktime_to_timespec64(start_time);
> +
> + /* Start can be up to eight seconds in the future */
> + ts.tv_sec %= 8;
What happens if base_time is specified as zero, or a small number that
only encodes a phase?
I would expect that the base time is advanced by an integer number of
cycles until it becomes in the immediate future, so that it can be
applied.
I don't think that's what happens right now.
If the start_time is 0.000000000, the cycle_time is 0.333333333, and now
is 8.125000000.
I believe that what would happen is:
- hellcreek_schedule_startable() says "yes, it's startable right away",
because base_time_ns - current_ns) is negative, and therefore also
smaller than 8 * NSEC_PER_SEC.
- hellcreek_switch_schedule() gets called with the 0.000000000 time, and
this start_time gets programmed right away into hardware.
What does the hardware do if it's given a schedule that begins at 0.000000000
when the current time is at 8.125000000?
I would expect that somebody (hardware or driver) advances the time
0.000000000 into the first time that is larger than 8.125000000, while
still remaining congruent to the original time in terms of phase offset.
I.e. advance the base time by 25 cycle times, to get
0.000000000 + 25 x 0.333333333 = 8.333333325 ns.
Then I would expect the schedule to start at 8.333333325 nanoseconds,
which is the first value immediately larger than "now" (8.125000000).
When you give the hardware the base_time of 0.000000000, is this what
happens? Is it equivalent to giving it 0.333333325?
> +
> + /* Start schedule at this point of time */
> + hellcreek_write(hellcreek, ts.tv_nsec & 0x0000ffff, TR_ESTWRL);
> + hellcreek_write(hellcreek, (ts.tv_nsec & 0xffff0000) >> 16, TR_ESTWRH);
> +
> + /* Arm timer, set seconds and switch schedule */
> + hellcreek_write(hellcreek, TR_ESTCMD_ESTARM | TR_ESTCMD_ESTSWCFG |
> + ((ts.tv_sec & TR_ESTCMD_ESTSEC_MASK) <<
> + TR_ESTCMD_ESTSEC_SHIFT), TR_ESTCMD);
> +}
> +
> +static struct hellcreek_schedule *
> +hellcreek_taprio_to_schedule(struct tc_taprio_qopt_offload *taprio)
> +{
> + struct hellcreek_schedule *schedule;
> + size_t i;
> +
> + /* Allocate some memory first */
> + schedule = kzalloc(sizeof(*schedule), GFP_KERNEL);
struct hellcreek_schedule has no added value on top of struct
tc_taprio_qopt_offload (except for _maybe_ that overrun_ignore, which
currently you don't use and could therefore remove - arguably the
overrun_ignore flag should be user-visible if it ever was to be
configured, and therefore my argument still stands).
The reason why I'm making the point, though, is that you don't need to
allocate extra memory if you use the plain struct tc_taprio_qopt_offload.
You can use taprio_offload_get() to increase the reference count on the
structure that was passed to you, use it for as long as you need, and
just call taprio_offload_free() when you're done with it.
> + if (!schedule)
> + return ERR_PTR(-ENOMEM);
> + schedule->entries = kcalloc(taprio->num_entries,
> + sizeof(*schedule->entries),
> + GFP_KERNEL);
> + if (!schedule->entries) {
> + kfree(schedule);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + /* Construct hellcreek schedule */
> + schedule->num_entries = taprio->num_entries;
> + schedule->base_time = taprio->base_time;
> +
> + for (i = 0; i < taprio->num_entries; ++i) {
> + const struct tc_taprio_sched_entry *t = &taprio->entries[i];
> + struct hellcreek_gcl_entry *k = &schedule->entries[i];
> +
> + k->interval = t->interval;
> + k->gate_states = t->gate_mask;
> + k->overrun_ignore = 0;
> +
> + /* Update complete cycle time */
> + schedule->cycle_time += t->interval;
You shouldn't need to do this, the cycle_time from struct
tc_taprio_qopt_offload should come properly populated. If it doesn't
it's a bug.
> + }
> +
> + return schedule;
> +}
> +
> +static void hellcreek_free_schedule(struct hellcreek *hellcreek, int port)
> +{
> + struct hellcreek_port *hellcreek_port = &hellcreek->ports[port];
> +
> + kfree(hellcreek_port->current_schedule->entries);
> + kfree(hellcreek_port->current_schedule);
> + hellcreek_port->current_schedule = NULL;
> +}
> +
> +static bool hellcreek_schedule_startable(struct hellcreek *hellcreek, int port)
> +{
> + struct hellcreek_port *hellcreek_port = &hellcreek->ports[port];
> + s64 base_time_ns, current_ns;
> +
> + /* The switch allows a schedule to be started only eight seconds within
> + * the future. Therefore, check the current PTP time if the schedule is
> + * startable or not.
> + */
The future of whom? I expect that TR_ESTWR is relative to the most
recent integer multiple of 8 seconds, and not relative to "now".
Otherwise you would never be able to phase-align taprio schedules with
other devices in the LAN.
Doesn't this mean that you need to be extra careful at modulo-8-seconds
wraparounds of the current PTP time?
> +
> + /* Use the "cached" time. That should be alright, as it's updated quite
> + * frequently in the PTP code.
> + */
> + mutex_lock(&hellcreek->ptp_lock);
> + current_ns = hellcreek->seconds * NSEC_PER_SEC + hellcreek->last_ts;
> + mutex_unlock(&hellcreek->ptp_lock);
> +
> + /* Calculate difference to admin base time */
> + base_time_ns = ktime_to_ns(hellcreek_port->current_schedule->base_time);
> +
> + if (base_time_ns - current_ns < (s64)8 * NSEC_PER_SEC)
> + return true;
> +
> + return false;
> +}
> +
> +static void hellcreek_start_schedule(struct hellcreek *hellcreek, int port)
> +{
> + struct hellcreek_port *hellcreek_port = &hellcreek->ports[port];
> +
> + /* First select port */
> + hellcreek_select_tgd(hellcreek, port);
> +
> + /* Set admin base time and switch schedule */
> + hellcreek_switch_schedule(hellcreek,
> + hellcreek_port->current_schedule->base_time);
> +
> + hellcreek_free_schedule(hellcreek, port);
> +
> + dev_dbg(hellcreek->dev, "ARMed EST timer for port %d\n",
Is ARM used as an acronym for something here? Why not Armed?
> + hellcreek_port->port);
> +}
> +
> +static void hellcreek_check_schedule(struct work_struct *work)
> +{
> + struct delayed_work *dw = to_delayed_work(work);
> + struct hellcreek_port *hellcreek_port;
> + struct hellcreek *hellcreek;
> + bool startable;
> +
> + hellcreek_port = dw_to_hellcreek_port(dw);
> + hellcreek = hellcreek_port->hellcreek;
> +
> + mutex_lock(&hellcreek->reg_lock);
> +
> + /* Check starting time */
> + startable = hellcreek_schedule_startable(hellcreek,
> + hellcreek_port->port);
> + if (startable) {
> + hellcreek_start_schedule(hellcreek, hellcreek_port->port);
> + mutex_unlock(&hellcreek->reg_lock);
> + return;
> + }
> +
> + mutex_unlock(&hellcreek->reg_lock);
> +
> + /* Reschedule */
> + schedule_delayed_work(&hellcreek_port->schedule_work,
> + HELLCREEK_SCHEDULE_PERIOD);
> +}
> +
> +static int hellcreek_port_set_schedule(struct dsa_switch *ds, int port,
> + struct tc_taprio_qopt_offload *taprio)
> +{
> + struct hellcreek *hellcreek = ds->priv;
> + struct hellcreek_port *hellcreek_port;
> + struct hellcreek_schedule *schedule;
> + bool startable;
> + u16 ctrl;
> +
> + hellcreek_port = &hellcreek->ports[port];
> +
> + /* Convert taprio data to hellcreek schedule */
> + schedule = hellcreek_taprio_to_schedule(taprio);
> + if (IS_ERR(schedule))
> + return PTR_ERR(schedule);
> +
> + dev_dbg(hellcreek->dev, "Configure traffic schedule on port %d\n",
> + port);
> +
> + /* First cancel delayed work */
> + cancel_delayed_work_sync(&hellcreek_port->schedule_work);
> +
> + mutex_lock(&hellcreek->reg_lock);
> +
> + if (hellcreek_port->current_schedule)
> + hellcreek_free_schedule(hellcreek, port);
> + hellcreek_port->current_schedule = schedule;
> +
> + /* Then select port */
> + hellcreek_select_tgd(hellcreek, port);
> +
> + /* Enable gating and set the admin state to forward everything in the
> + * mean time
> + */
> + ctrl = (0xff << TR_TGDCTRL_ADMINGATESTATES_SHIFT) | TR_TGDCTRL_GATE_EN;
> + hellcreek_write(hellcreek, ctrl, TR_TGDCTRL);
Hmm, "in the meantime"? When do these admin gate states become effective?
If possible, I expect that the currently operational schedule remains
operational exactly until the base_time of the newly installed one,
minus a possible cycle_time_extension.
> +
> + /* Cancel pending schedule */
> + hellcreek_write(hellcreek, 0x00, TR_ESTCMD);
> +
> + /* Setup a new schedule */
> + hellcreek_setup_gcl(hellcreek, port, schedule);
> +
> + /* Configure cycle time */
> + hellcreek_set_cycle_time(hellcreek, schedule);
As a general comment, if the hardware doesn't support the cycle time
extension when switching schedules, then you should NACK a non-zero
passed argument there.
> +
> + /* Check starting time */
> + startable = hellcreek_schedule_startable(hellcreek, port);
> + if (startable) {
> + hellcreek_start_schedule(hellcreek, port);
> + mutex_unlock(&hellcreek->reg_lock);
> + return 0;
> + }
> +
> + mutex_unlock(&hellcreek->reg_lock);
> +
> + /* Schedule periodic schedule check */
> + schedule_delayed_work(&hellcreek_port->schedule_work,
> + HELLCREEK_SCHEDULE_PERIOD);
> +
> + return 0;
> +}
> +
> +static int hellcreek_port_del_schedule(struct dsa_switch *ds, int port)
> +{
> + struct hellcreek *hellcreek = ds->priv;
> + struct hellcreek_port *hellcreek_port;
> +
> + hellcreek_port = &hellcreek->ports[port];
> +
> + dev_dbg(hellcreek->dev, "Remove traffic schedule on port %d\n", port);
> +
> + /* First cancel delayed work */
> + cancel_delayed_work_sync(&hellcreek_port->schedule_work);
> +
> + mutex_lock(&hellcreek->reg_lock);
> +
> + if (hellcreek_port->current_schedule)
> + hellcreek_free_schedule(hellcreek, port);
> +
> + /* Then select port */
> + hellcreek_select_tgd(hellcreek, port);
> +
> + /* Disable gating and return to regular switching flow */
> + hellcreek_write(hellcreek, 0xff << TR_TGDCTRL_ADMINGATESTATES_SHIFT,
> + TR_TGDCTRL);
> +
> + mutex_unlock(&hellcreek->reg_lock);
> +
> + return 0;
> +}
> +
> +static int hellcreek_port_setup_tc(struct dsa_switch *ds, int port,
> + enum tc_setup_type type, void *type_data)
> +{
> + struct tc_taprio_qopt_offload *taprio = type_data;
> + struct hellcreek *hellcreek = ds->priv;
> +
> + if (type != TC_SETUP_QDISC_TAPRIO)
> + return -EOPNOTSUPP;
> +
> + /* Does this hellcreek version support Qbv in hardware? */
> + if (!hellcreek->pdata->qbv_support)
> + return -EOPNOTSUPP;
> +
> + if (taprio->enable)
> + return hellcreek_port_set_schedule(ds, port, taprio);
> +
> + return hellcreek_port_del_schedule(ds, port);
> +}
> +
> static const struct dsa_switch_ops hellcreek_ds_ops = {
> .get_ethtool_stats = hellcreek_get_ethtool_stats,
> .get_sset_count = hellcreek_get_sset_count,
> @@ -1153,6 +1463,7 @@ static const struct dsa_switch_ops hellcreek_ds_ops = {
> .port_hwtstamp_get = hellcreek_port_hwtstamp_get,
> .port_prechangeupper = hellcreek_port_prechangeupper,
> .port_rxtstamp = hellcreek_port_rxtstamp,
> + .port_setup_tc = hellcreek_port_setup_tc,
> .port_stp_state_set = hellcreek_port_stp_state_set,
> .port_txtstamp = hellcreek_port_txtstamp,
> .port_vlan_add = hellcreek_vlan_add,
> @@ -1208,6 +1519,9 @@ static int hellcreek_probe(struct platform_device *pdev)
>
> port->hellcreek = hellcreek;
> port->port = i;
> +
> + INIT_DELAYED_WORK(&port->schedule_work,
> + hellcreek_check_schedule);
> }
>
> mutex_init(&hellcreek->reg_lock);
> diff --git a/drivers/net/dsa/hirschmann/hellcreek.h b/drivers/net/dsa/hirschmann/hellcreek.h
> index e81781ebc31c..7ffb1b33ff72 100644
> --- a/drivers/net/dsa/hirschmann/hellcreek.h
> +++ b/drivers/net/dsa/hirschmann/hellcreek.h
> @@ -213,6 +213,20 @@ struct hellcreek_counter {
> const char *name;
> };
>
> +struct hellcreek_gcl_entry {
> + u32 interval;
> + u8 gate_states;
> + bool overrun_ignore;
> +};
> +
> +struct hellcreek_schedule {
> + struct hellcreek_gcl_entry *entries;
> + size_t num_entries;
> + ktime_t base_time;
> + u32 cycle_time;
> + int port;
You don't need/use the "port" member.
> +};
> +
> struct hellcreek;
>
> /* State flags for hellcreek_port_hwtstamp::state */
> @@ -246,6 +260,10 @@ struct hellcreek_port {
>
> /* Per-port timestamping resources */
> struct hellcreek_port_hwtstamp port_hwtstamp;
> +
> + /* Per-port Qbv schedule information */
> + struct hellcreek_schedule *current_schedule;
> + struct delayed_work schedule_work;
> };
>
> struct hellcreek_fdb_entry {
> @@ -283,4 +301,8 @@ struct hellcreek {
> size_t fdb_entries;
> };
>
> +#define HELLCREEK_SCHEDULE_PERIOD (2 * HZ)
Is there a risk if you schedule rarer than this? The hellcreek->seconds
value is no longer accurate enough?
> +#define dw_to_hellcreek_port(dw) \
> + container_of(dw, struct hellcreek_port, schedule_work)
> +
> #endif /* _HELLCREEK_H_ */
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH net-next 1/2] net: dsa: tag_hellcreek: Cleanup includes
From: Vladimir Oltean @ 2020-11-22 23:09 UTC (permalink / raw)
To: Kurt Kanzenbach
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
Jakub Kicinski, netdev
In-Reply-To: <20201121114455.22422-2-kurt@linutronix.de>
On Sat, Nov 21, 2020 at 12:44:54PM +0100, Kurt Kanzenbach wrote:
> Remove unused and add needed includes. No functional change.
>
> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: dsa: hellcreek: Don't print error message on defer
From: Vladimir Oltean @ 2020-11-22 23:10 UTC (permalink / raw)
To: Kurt Kanzenbach
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
Jakub Kicinski, netdev
In-Reply-To: <20201121114455.22422-3-kurt@linutronix.de>
On Sat, Nov 21, 2020 at 12:44:55PM +0100, Kurt Kanzenbach wrote:
> When DSA is not loaded when the driver is probed an error message is
> printed. But, that's not really an error, just a defer. Use dev_err_probe()
> instead.
>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
^ permalink raw reply
* Re: [PATCH net-next 2/3] net: dsa: add Arrow SpeedChips XRS700x driver
From: Vladimir Oltean @ 2020-11-22 23:39 UTC (permalink / raw)
To: George McCollister
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller ,
netdev, devicetree
In-Reply-To: <20201120181627.21382-3-george.mccollister@gmail.com>
Hi George,
On Fri, Nov 20, 2020 at 12:16:26PM -0600, George McCollister wrote:
> Add a driver with initial support for the Arrow SpeedChips XRS7000
> series of gigabit Ethernet switch chips which are typically used in
> critical networking applications.
>
> The switches have up to three RGMII ports and one RMII port.
> Management to the switches can be performed over i2c or mdio.
>
> Support for advanced features such as PTP and
> HSR/PRP (IEC 62439-3 Clause 5 & 4) is not included in this patch and
> may be added at a later date.
>
> Signed-off-by: George McCollister <george.mccollister@gmail.com>
> ---
> drivers/net/dsa/Kconfig | 26 ++
> drivers/net/dsa/Makefile | 3 +
> drivers/net/dsa/xrs700x.c | 529 +++++++++++++++++++++++++++++++++++++++++
> drivers/net/dsa/xrs700x.h | 27 +++
> drivers/net/dsa/xrs700x_i2c.c | 148 ++++++++++++
> drivers/net/dsa/xrs700x_mdio.c | 160 +++++++++++++
> drivers/net/dsa/xrs700x_reg.h | 205 ++++++++++++++++
How much code do you plan to add to this driver? If it's going to
include IEEE 1588 and HSR/PRP offloading, would it make sense to put its
source code in a new folder now, to avoid doing that later?
> 7 files changed, 1098 insertions(+)
> create mode 100644 drivers/net/dsa/xrs700x.c
> create mode 100644 drivers/net/dsa/xrs700x.h
> create mode 100644 drivers/net/dsa/xrs700x_i2c.c
> create mode 100644 drivers/net/dsa/xrs700x_mdio.c
> create mode 100644 drivers/net/dsa/xrs700x_reg.h
>
> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> index f6a0488589fc..e5ec3c602bcb 100644
> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -134,4 +134,30 @@ config NET_DSA_VITESSE_VSC73XX_PLATFORM
> This enables support for the Vitesse VSC7385, VSC7388, VSC7395
> and VSC7398 SparX integrated ethernet switches, connected over
> a CPU-attached address bus and work in memory-mapped I/O mode.
> +
> +config NET_DSA_XRS700X
> + tristate
> + depends on NET_DSA
> + select NET_DSA_TAG_XRS700X
> + select REGMAP
> + help
> + This enables support for Arrow SpeedChips XRS7003/7004 gigabit
> + Ethernet switches.
> +
> +config NET_DSA_XRS700X_I2C
> + tristate "Arrow XRS7000X series switch in I2C mode"
> + depends on NET_DSA && I2C
> + select NET_DSA_XRS700X
> + select REGMAP_I2C
> + help
> + Enable I2C support for Arrow SpeedChips XRS7003/7004 gigabit Ethernet
> + switches.
> +
> +config NET_DSA_XRS700X_MDIO
> + tristate "Arrow XRS7000X series switch in MDIO mode"
> + depends on NET_DSA
> + select NET_DSA_XRS700X
> + help
> + Enable MDIO support for Arrow SpeedChips XRS7003/7004 gigabit Ethernet
> + switches.
> endmenu
> diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
> index a84adb140a04..4528d6b57fc8 100644
> --- a/drivers/net/dsa/Makefile
> +++ b/drivers/net/dsa/Makefile
> @@ -17,6 +17,9 @@ obj-$(CONFIG_NET_DSA_SMSC_LAN9303_MDIO) += lan9303_mdio.o
> obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX) += vitesse-vsc73xx-core.o
> obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_PLATFORM) += vitesse-vsc73xx-platform.o
> obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_SPI) += vitesse-vsc73xx-spi.o
> +obj-$(CONFIG_NET_DSA_XRS700X) += xrs700x.o
> +obj-$(CONFIG_NET_DSA_XRS700X_I2C) += xrs700x_i2c.o
> +obj-$(CONFIG_NET_DSA_XRS700X_MDIO) += xrs700x_mdio.o
> obj-y += b53/
> obj-y += hirschmann/
> obj-y += microchip/
> diff --git a/drivers/net/dsa/xrs700x.c b/drivers/net/dsa/xrs700x.c
> new file mode 100644
> index 000000000000..6cef3b534d5d
> --- /dev/null
> +++ b/drivers/net/dsa/xrs700x.c
> @@ -0,0 +1,529 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 NovaTech LLC
> + * George McCollister <george.mccollister@gmail.com>
> + */
> +
> +#include <net/dsa.h>
> +#include <linux/if_bridge.h>
> +#include "xrs700x.h"
> +#include "xrs700x_reg.h"
> +
> +#define XRS700X_MIB_INTERVAL msecs_to_jiffies(30000)
> +
> +#define XRS7003E_ID 0x100
> +#define XRS7003F_ID 0x101
> +#define XRS7004E_ID 0x200
> +#define XRS7004F_ID 0x201
> +
> +struct xrs700x_model {
> + unsigned int id;
> + const char *name;
> + size_t num_ports;
> +};
> +
> +static const struct xrs700x_model xrs700x_models[] = {
> + {XRS7003E_ID, "XRS7003E", 3},
> + {XRS7003F_ID, "XRS7003F", 3},
> + {XRS7004E_ID, "XRS7004E", 4},
> + {XRS7004F_ID, "XRS7004F", 4},
> +};
> +
> +struct xrs700x_mib {
> + unsigned int offset;
> + const char *name;
> +};
> +
> +static const struct xrs700x_mib xrs700x_mibs[] = {
> + {XRS_RX_GOOD_OCTETS_L(0), "rx_good_octets"},
> + {XRS_RX_BAD_OCTETS_L(0), "rx_bad_octets"},
> + {XRS_RX_UNICAST_L(0), "rx_unicast"},
> + {XRS_RX_BROADCAST_L(0), "rx_broadcast"},
> + {XRS_RX_MULTICAST_L(0), "rx_multicast"},
> + {XRS_RX_UNDERSIZE_L(0), "rx_undersize"},
> + {XRS_RX_FRAGMENTS_L(0), "rx_fragments"},
> + {XRS_RX_OVERSIZE_L(0), "rx_oversize"},
> + {XRS_RX_JABBER_L(0), "rx_jabber"},
> + {XRS_RX_ERR_L(0), "rx_err"},
> + {XRS_RX_CRC_L(0), "rx_crc"},
> + {XRS_RX_64_L(0), "rx_64"},
> + {XRS_RX_65_127_L(0), "rx_65_127"},
> + {XRS_RX_128_255_L(0), "rx_128_255"},
> + {XRS_RX_256_511_L(0), "rx_256_511"},
> + {XRS_RX_512_1023_L(0), "rx_512_1023"},
> + {XRS_RX_1024_1536_L(0), "rx_1024_1536"},
Uh-oh, Jakub might not like these RMON counters being exposed to
ethtool. See:
https://patchwork.kernel.org/project/netdevbpf/patch/20201115073533.1366-1-o.rempel@pengutronix.de/
> + {XRS_RX_HSR_PRP_L(0), "rx_hsr_prp"},
> + {XRS_RX_WRONGLAN_L(0), "rx_wronglan"},
> + {XRS_RX_DUPLICATE_L(0), "rx_duplicate"},
> + {XRS_TX_OCTETS_L(0), "tx_octets"},
> + {XRS_TX_UNICAST_L(0), "tx_unicast"},
> + {XRS_TX_BROADCAST_L(0), "tx_broadcast"},
> + {XRS_TX_MULTICAST_L(0), "tx_multicast"},
> + {XRS_TX_HSR_PRP_L(0), "tx_hsr_prp"},
> + {XRS_PRIQ_DROP_L(0), "priq_drop"},
> + {XRS_EARLY_DROP_L(0), "early_drop"},
> +};
> +
> +static void xrs700x_get_strings(struct dsa_switch *ds, int port,
> + u32 stringset, uint8_t *data)
> +{
> + int i;
> +
> + if (stringset != ETH_SS_STATS)
> + return;
> +
> + for (i = 0; i < ARRAY_SIZE(xrs700x_mibs); i++) {
> + strlcpy(data, xrs700x_mibs[i].name, ETH_GSTRING_LEN);
> + data += ETH_GSTRING_LEN;
> + }
> +}
> +
> +static int xrs700x_get_sset_count(struct dsa_switch *ds, int port, int sset)
> +{
> + if (sset != ETH_SS_STATS)
> + return -EOPNOTSUPP;
> +
> + return ARRAY_SIZE(xrs700x_mibs);
> +}
> +
> +static void xrs700x_read_port_counters(struct xrs700x *priv, int port)
> +{
> + int i;
> + struct xrs700x_port *p = &priv->ports[port];
> +
> + mutex_lock(&p->mib_mutex);
> +
> + /* Capture counter values */
> + regmap_write(priv->regmap, XRS_CNT_CTRL(port), 1);
> +
> + for (i = 0; i < ARRAY_SIZE(xrs700x_mibs); i++) {
> + unsigned int high = 0, low = 0, reg;
> +
> + reg = xrs700x_mibs[i].offset + XRS_PORT_OFFSET * port;
> + regmap_read(priv->regmap, reg, &low);
> + regmap_read(priv->regmap, reg + 2, &high);
> +
> + p->mib_data[i] += (high << 16) | low;
> + }
> +
> + mutex_unlock(&p->mib_mutex);
> +}
> +
> +static void xrs700x_mib_work(struct work_struct *work)
> +{
> + struct xrs700x *priv = container_of(work, struct xrs700x,
> + mib_work.work);
> + int i;
> +
> + for (i = 0; i < priv->ds->num_ports; i++)
> + xrs700x_read_port_counters(priv, i);
> +
> + schedule_delayed_work(&priv->mib_work, XRS700X_MIB_INTERVAL);
> +}
> +
> +static void xrs700x_get_ethtool_stats(struct dsa_switch *ds, int port,
> + uint64_t *data)
> +{
> + struct xrs700x *priv = ds->priv;
> + struct xrs700x_port *p = &priv->ports[port];
> +
> + xrs700x_read_port_counters(priv, port);
> +
> + mutex_lock(&p->mib_mutex);
> + memcpy(data, p->mib_data, sizeof(*data) * ARRAY_SIZE(xrs700x_mibs));
> + mutex_unlock(&p->mib_mutex);
> +}
> +
> +static int xrs700x_setup_regmap_range(struct xrs700x *priv)
> +{
> + struct reg_field ps_forward = REG_FIELD_ID(XRS_PORT_STATE(0), 0, 1,
> + priv->ds->num_ports,
> + XRS_PORT_OFFSET);
Oh, hey, another REG_FIELD_ID user!
> + struct reg_field ps_management = REG_FIELD_ID(XRS_PORT_STATE(0), 2, 3,
> + priv->ds->num_ports,
> + XRS_PORT_OFFSET);
> + struct reg_field ps_sel_speed = REG_FIELD_ID(XRS_PORT_STATE(0), 4, 9,
> + priv->ds->num_ports,
> + XRS_PORT_OFFSET);
> + struct reg_field ps_cur_speed = REG_FIELD_ID(XRS_PORT_STATE(0), 10, 11,
> + priv->ds->num_ports,
> + XRS_PORT_OFFSET);
> +
> + priv->ps_forward = devm_regmap_field_alloc(priv->dev, priv->regmap,
> + ps_forward);
> + if (IS_ERR(priv->ps_forward))
> + return PTR_ERR(priv->ps_forward);
> +
> + priv->ps_management = devm_regmap_field_alloc(priv->dev, priv->regmap,
> + ps_management);
> + if (IS_ERR(priv->ps_management))
> + return PTR_ERR(priv->ps_management);
> +
> + priv->ps_sel_speed = devm_regmap_field_alloc(priv->dev, priv->regmap,
> + ps_sel_speed);
> + if (IS_ERR(priv->ps_sel_speed))
> + return PTR_ERR(priv->ps_sel_speed);
> +
> + priv->ps_cur_speed = devm_regmap_field_alloc(priv->dev, priv->regmap,
> + ps_cur_speed);
> + if (IS_ERR(priv->ps_cur_speed))
> + return PTR_ERR(priv->ps_cur_speed);
Should you try to automate allocating these? You might get tired of
adding and adding and adding to this function really quick. You might
get some inspiration from ocelot_regfields_init() and that driver's use
of regmap in general.
> +
> + return 0;
> +}
> +
> +static enum dsa_tag_protocol xrs700x_get_tag_protocol(struct dsa_switch *ds,
> + int port,
> + enum dsa_tag_protocol m)
> +{
> + return DSA_TAG_PROTO_XRS700X;
> +}
> +
> +static int xrs700x_reset(struct dsa_switch *ds)
> +{
> + struct xrs700x *priv = ds->priv;
> + int ret;
> + unsigned int val;
> +
> + ret = regmap_write(priv->regmap, XRS_GENERAL, XRS_GENERAL_RESET);
> + if (ret)
> + goto error;
> +
> + ret = regmap_read_poll_timeout(priv->regmap, XRS_GENERAL,
> + val, !(val & XRS_GENERAL_RESET),
> + 10, 1000);
> +error:
> + if (ret) {
> + dev_err_ratelimited(priv->dev, "error resetting switch: %d\n",
> + ret);
> + }
> +
> + return ret;
> +}
> +
> +static void xrs700x_port_stp_state_set(struct dsa_switch *ds, int port,
> + u8 state)
> +{
> + struct xrs700x *priv = ds->priv;
> + unsigned int val;
> +
> + switch (state) {
> + case BR_STATE_DISABLED:
> + val = XRS_PORT_DISABLED;
> + break;
> + case BR_STATE_LISTENING:
> + val = XRS_PORT_DISABLED;
> + break;
> + case BR_STATE_LEARNING:
> + val = XRS_PORT_LEARNING;
> + break;
> + case BR_STATE_FORWARDING:
> + val = XRS_PORT_FORWARDING;
> + break;
> + case BR_STATE_BLOCKING:
> + val = XRS_PORT_DISABLED;
Why not just put BR_STATE_DISABLED and BR_STATE_BLOCKING one under the
other?
case BR_STATE_BLOCKING:
case BR_STATE_DISABLED:
val = XRS_PORT_DISABLED;
> + break;
> + default:
> + dev_err(ds->dev, "invalid STP state: %d\n", state);
> + return;
> + }
> +
> + regmap_fields_write(priv->ps_forward, port, val);
> +
> + dev_dbg_ratelimited(priv->dev, "%s - port: %d, state: %u, val: 0x%x\n",
> + __func__, port, state, val);
> +}
> +
> +static int xrs700x_port_setup(struct dsa_switch *ds, int port)
> +{
> + struct xrs700x *priv = ds->priv;
> + bool cpu_port = dsa_is_cpu_port(ds, port);
Reverse Christmas tree notation please.
> + unsigned int val;
Ugh, you couldn't have initialized this with zero here? It looks ugly
putting that in the for loop.
> + int ret, i;
> +
> + xrs700x_port_stp_state_set(ds, port, BR_STATE_DISABLED);
> +
> + /* Disable forwarding to non-CPU ports */
> + for (val = 0, i = 0; i < ds->num_ports; i++) {
> + if (!dsa_is_cpu_port(ds, i))
> + val |= BIT(i);
> + }
> +
> + ret = regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port), val);
> + if (ret)
> + return ret;
> +
> + val = cpu_port ? XRS_PORT_MODE_MANAGEMENT : XRS_PORT_MODE_NORMAL;
> + ret = regmap_fields_write(priv->ps_management, port, val);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int xrs700x_setup(struct dsa_switch *ds)
> +{
> + struct xrs700x *priv = ds->priv;
> + int ret, i;
> +
> + ret = xrs700x_reset(ds);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < ds->num_ports; i++) {
> + ret = xrs700x_port_setup(ds, i);
> + if (ret)
> + return ret;
> + }
> +
> + schedule_delayed_work(&priv->mib_work, XRS700X_MIB_INTERVAL);
> +
> + return 0;
> +}
> +
> +static void xrs700x_phylink_validate(struct dsa_switch *ds, int port,
> + unsigned long *supported,
> + struct phylink_link_state *state)
> +{
> + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +
> + switch (port) {
> + case 0:
> + break;
> + case 1:
> + case 2:
> + case 3:
> + phylink_set(mask, 1000baseT_Full);
> + break;
> + default:
> + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> + dev_err(ds->dev, "Unsupported port: %i\n", port);
> + return;
> + }
> +
> + phylink_set_port_modes(mask);
> +
> + /* The switch only supports full duplex. */
> + phylink_set(mask, 10baseT_Full);
> + phylink_set(mask, 100baseT_Full);
> +
> + bitmap_and(supported, supported, mask,
> + __ETHTOOL_LINK_MODE_MASK_NBITS);
> + bitmap_and(state->advertising, state->advertising, mask,
> + __ETHTOOL_LINK_MODE_MASK_NBITS);
> +}
> +
> +static void xrs700x_phylink_mac_config(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + const struct phylink_link_state *state)
As far as I understand phylink, you should be programming the link speed
of the RGMII/RMII MAC from the .mac_link_up callback.
> +{
> + struct xrs700x *priv = ds->priv;
> + unsigned int val;
> +
> + switch (state->speed) {
> + case SPEED_1000:
> + val = XRS_PORT_SPEED_1000;
> + break;
> + case SPEED_100:
> + val = XRS_PORT_SPEED_100;
> + break;
> + case SPEED_10:
> + val = XRS_PORT_SPEED_10;
> + break;
> + default:
> + return;
> + }
> +
> + regmap_fields_write(priv->ps_sel_speed, port, val);
> +
> + dev_dbg_ratelimited(priv->dev, "%s: port: %d mode: %u speed: %u\n",
> + __func__, port, mode, state->speed);
> +}
> +
> +static int xrs700x_bridge_join(struct dsa_switch *ds, int port,
> + struct net_device *bridge)
> +{
> + struct xrs700x *priv = ds->priv;
> + unsigned int i, ret, mask = 0;
> +
> + for (i = 0; i < ds->num_ports; i++) {
> + if (dsa_to_port(ds, i)->bridge_dev == bridge ||
> + dsa_is_cpu_port(ds, i))
> + continue;
> +
> + mask |= BIT(i);
> + }
> +
> + dev_dbg(priv->dev, "%s: port: %d mask: 0x%x\n", __func__,
> + port, mask);
> +
> + for (i = 0; i < ds->num_ports; i++) {
> + if (dsa_to_port(ds, i)->bridge_dev != bridge)
> + continue;
> +
> + ret = regmap_write(priv->regmap, XRS_PORT_FWD_MASK(i), mask);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void xrs700x_bridge_leave(struct dsa_switch *ds, int port,
> + struct net_device *bridge)
Don't be lazy, you can avoid copy-pasting the implementation for this
one...
> +{
> + struct xrs700x *priv = ds->priv;
> + unsigned int i, cpu_mask = 0, mask = 0;
> +
> + for (i = 0; i < ds->num_ports; i++) {
> + if (dsa_is_cpu_port(ds, i))
> + continue;
> +
> + cpu_mask |= BIT(i);
> +
> + if (dsa_to_port(ds, i)->bridge_dev == bridge)
> + continue;
> +
> + mask |= BIT(i);
> + }
> +
> + dev_dbg(priv->dev, "%s: port: %d mask: 0x%x\n", __func__,
> + port, mask);
> +
> + for (i = 0; i < ds->num_ports; i++) {
> + if (dsa_to_port(ds, i)->bridge_dev != bridge)
> + continue;
> +
> + regmap_write(priv->regmap, XRS_PORT_FWD_MASK(i), mask);
> + }
> +
> + regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port), cpu_mask);
> +}
> +
> +static const struct dsa_switch_ops xrs700x_ops = {
> + .get_tag_protocol = xrs700x_get_tag_protocol,
> + .setup = xrs700x_setup,
> + .port_stp_state_set = xrs700x_port_stp_state_set,
> + .phylink_validate = xrs700x_phylink_validate,
> + .phylink_mac_config = xrs700x_phylink_mac_config,
> + .get_strings = xrs700x_get_strings,
> + .get_sset_count = xrs700x_get_sset_count,
> + .get_ethtool_stats = xrs700x_get_ethtool_stats,
> + .port_bridge_join = xrs700x_bridge_join,
> + .port_bridge_leave = xrs700x_bridge_leave,
> +};
> +
> +static int xrs700x_detect(struct xrs700x *dev)
> +{
> + int i, ret;
> + unsigned int id;
> +
> + ret = regmap_read(dev->regmap, XRS_DEV_ID0, &id);
> + if (ret) {
> + dev_err(dev->dev, "error %d while reading switch id.\n",
> + ret);
> + return ret;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(xrs700x_models); i++) {
> + if (xrs700x_models[i].id == id) {
> + dev->ds->num_ports = xrs700x_models[i].num_ports;
> + dev_info(dev->dev, "%s detected.\n",
> + xrs700x_models[i].name);
> + return 0;
> + }
> + }
> +
> + dev_err(dev->dev, "unknown switch id 0x%x.\n", id);
> +
> + return -ENODEV;
> +}
> +
> +struct xrs700x *xrs700x_switch_alloc(struct device *base, void *priv)
> +{
> + struct dsa_switch *ds;
> + struct xrs700x *dev;
> +
> + ds = devm_kzalloc(base, sizeof(*ds), GFP_KERNEL);
> + if (!ds)
> + return NULL;
> +
> + ds->dev = base;
> + ds->num_ports = DSA_MAX_PORTS;
Why so many?
> +
> + dev = devm_kzalloc(base, sizeof(*dev), GFP_KERNEL);
> + if (!dev)
> + return NULL;
> +
> + INIT_DELAYED_WORK(&dev->mib_work, xrs700x_mib_work);
> +
> + ds->ops = &xrs700x_ops;
> + ds->priv = dev;
> + dev->dev = base;
> +
> + dev->ds = ds;
> + dev->priv = priv;
> +
> + return dev;
> +}
> +EXPORT_SYMBOL(xrs700x_switch_alloc);
> +
> +static int xrs700x_alloc_port_mib(struct xrs700x *dev, int port)
> +{
> + struct xrs700x_port *p = &dev->ports[port];
> + size_t mib_size = sizeof(*p->mib_data) * ARRAY_SIZE(xrs700x_mibs);
> +
> + p->mib_data = devm_kzalloc(dev->dev, mib_size, GFP_KERNEL);
> + if (!p->mib_data)
> + return -ENOMEM;
> +
> + mutex_init(&p->mib_mutex);
> +
> + return 0;
> +}
> +
> +int xrs700x_switch_register(struct xrs700x *dev)
> +{
> + int ret;
> + int i;
> +
> + ret = xrs700x_detect(dev);
> + if (ret)
> + return ret;
> +
> + ret = xrs700x_setup_regmap_range(dev);
> + if (ret)
> + return ret;
> +
> + dev->ports = devm_kzalloc(dev->dev,
> + sizeof(*dev->ports) * dev->ds->num_ports,
> + GFP_KERNEL);
> + if (!dev->ports)
> + return -ENOMEM;
> +
> + for (i = 0; i < dev->ds->num_ports; i++) {
> + ret = xrs700x_alloc_port_mib(dev, i);
> + if (ret)
> + return ret;
> + }
> +
> + ret = dsa_register_switch(dev->ds);
> +
> + if (ret)
> + cancel_delayed_work_sync(&dev->mib_work);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(xrs700x_switch_register);
> +
> +void xrs700x_switch_remove(struct xrs700x *dev)
> +{
> + cancel_delayed_work_sync(&dev->mib_work);
> +
> + dsa_unregister_switch(dev->ds);
> +}
> +EXPORT_SYMBOL(xrs700x_switch_remove);
> +
> +MODULE_AUTHOR("George McCollister <george.mccollister@gmail.com>");
> +MODULE_DESCRIPTION("Arrow SpeedChips XRS700x DSA driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/net/dsa/xrs700x.h b/drivers/net/dsa/xrs700x.h
> new file mode 100644
> index 000000000000..53acf4359477
> --- /dev/null
> +++ b/drivers/net/dsa/xrs700x.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#include <linux/device.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/workqueue.h>
> +
> +struct xrs700x_port {
> + struct mutex mib_mutex; /* protects mib_data */
> + uint64_t *mib_data;
> +};
> +
> +struct xrs700x {
> + struct dsa_switch *ds;
> + struct device *dev;
> + void *priv;
> + struct regmap *regmap;
> + struct regmap_field *ps_forward;
> + struct regmap_field *ps_management;
> + struct regmap_field *ps_sel_speed;
> + struct regmap_field *ps_cur_speed;
> + struct delayed_work mib_work;
> + struct xrs700x_port *ports;
> +};
> +
> +struct xrs700x *xrs700x_switch_alloc(struct device *base, void *priv);
> +int xrs700x_switch_register(struct xrs700x *dev);
> +void xrs700x_switch_remove(struct xrs700x *dev);
> diff --git a/drivers/net/dsa/xrs700x_i2c.c b/drivers/net/dsa/xrs700x_i2c.c
> new file mode 100644
> index 000000000000..30f6c5ce825b
> --- /dev/null
> +++ b/drivers/net/dsa/xrs700x_i2c.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 NovaTech LLC
> + * George McCollister <george.mccollister@gmail.com>
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include "xrs700x.h"
> +#include "xrs700x_reg.h"
> +
> +static int xrs700x_i2c_reg_read(void *context, unsigned int reg,
> + unsigned int *val)
> +{
> + int ret;
> + unsigned char buf[4];
> + struct device *dev = context;
> + struct i2c_client *i2c = to_i2c_client(dev);
Please sort variable declaration in the order of decreasing line length.
> +
> + buf[0] = reg >> 23 & 0xff;
> + buf[1] = reg >> 15 & 0xff;
> + buf[2] = reg >> 7 & 0xff;
> + buf[3] = (reg & 0x7f) << 1;
> +
> + ret = i2c_master_send(i2c, buf, sizeof(buf));
> + if (ret < 0) {
> + dev_err(dev, "xrs i2c_master_send returned %d\n", ret);
> + return ret;
> + }
> +
> + ret = i2c_master_recv(i2c, buf, 2);
> + if (ret < 0) {
> + dev_err(dev, "xrs i2c_master_recv returned %d\n", ret);
> + return ret;
> + }
> +
> + *val = buf[0] << 8 | buf[1];
> +
> + return 0;
> +}
^ permalink raw reply
* [PATCH net-next 0/2] net: Constify static qmi structs
From: Rikard Falkeborn @ 2020-11-22 23:40 UTC (permalink / raw)
To: David S. Miller, Jakub Kicinski
Cc: Alex Elder, Kalle Valo, netdev, linux-kernel, ath10k,
linux-wireless, Rikard Falkeborn
Constify a couple of static qmi_ops and qmi_msg_handler structs that are
never modified.
Rikard Falkeborn (2):
soc: qcom: ipa: Constify static qmi structs
ath10k: Constify static qmi structs
drivers/net/ipa/ipa_qmi.c | 8 ++++----
drivers/net/wireless/ath/ath10k/qmi.c | 4 ++--
2 files changed, 6 insertions(+), 6 deletions(-)
--
2.29.2
^ permalink raw reply
* [PATCH net-next 1/2] soc: qcom: ipa: Constify static qmi structs
From: Rikard Falkeborn @ 2020-11-22 23:40 UTC (permalink / raw)
To: David S. Miller, Jakub Kicinski
Cc: Alex Elder, Kalle Valo, netdev, linux-kernel, ath10k,
linux-wireless, Rikard Falkeborn
In-Reply-To: <20201122234031.33432-1-rikard.falkeborn@gmail.com>
These are only used as input arguments to qmi_handle_init() which
accepts const pointers to both qmi_ops and qmi_msg_handler. Make them
const to allow the compiler to put them in read-only memory.
Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
---
drivers/net/ipa/ipa_qmi.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ipa/ipa_qmi.c b/drivers/net/ipa/ipa_qmi.c
index 5090f0f923ad..d2c3f273c233 100644
--- a/drivers/net/ipa/ipa_qmi.c
+++ b/drivers/net/ipa/ipa_qmi.c
@@ -168,7 +168,7 @@ static void ipa_server_bye(struct qmi_handle *qmi, unsigned int node)
ipa_qmi->indication_sent = false;
}
-static struct qmi_ops ipa_server_ops = {
+static const struct qmi_ops ipa_server_ops = {
.bye = ipa_server_bye,
};
@@ -234,7 +234,7 @@ static void ipa_server_driver_init_complete(struct qmi_handle *qmi,
}
/* The server handles two request message types sent by the modem. */
-static struct qmi_msg_handler ipa_server_msg_handlers[] = {
+static const struct qmi_msg_handler ipa_server_msg_handlers[] = {
{
.type = QMI_REQUEST,
.msg_id = IPA_QMI_INDICATION_REGISTER,
@@ -261,7 +261,7 @@ static void ipa_client_init_driver(struct qmi_handle *qmi,
}
/* The client handles one response message type sent by the modem. */
-static struct qmi_msg_handler ipa_client_msg_handlers[] = {
+static const struct qmi_msg_handler ipa_client_msg_handlers[] = {
{
.type = QMI_RESPONSE,
.msg_id = IPA_QMI_INIT_DRIVER,
@@ -463,7 +463,7 @@ ipa_client_new_server(struct qmi_handle *qmi, struct qmi_service *svc)
return 0;
}
-static struct qmi_ops ipa_client_ops = {
+static const struct qmi_ops ipa_client_ops = {
.new_server = ipa_client_new_server,
};
--
2.29.2
^ permalink raw reply related
* [PATCH net-next 2/2] ath10k: Constify static qmi structs
From: Rikard Falkeborn @ 2020-11-22 23:40 UTC (permalink / raw)
To: David S. Miller, Jakub Kicinski
Cc: Alex Elder, Kalle Valo, netdev, linux-kernel, ath10k,
linux-wireless, Rikard Falkeborn
In-Reply-To: <20201122234031.33432-1-rikard.falkeborn@gmail.com>
qmi_msg_handler[] and ath10k_qmi_ops are only used as input arguments
to qmi_handle_init() which accepts const pointers to both qmi_ops and
qmi_msg_handler. Make them const to allow the compiler to put them in
read-only memory.
Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
---
drivers/net/wireless/ath/ath10k/qmi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/qmi.c b/drivers/net/wireless/ath/ath10k/qmi.c
index ae6b1f402adf..07e478f9a808 100644
--- a/drivers/net/wireless/ath/ath10k/qmi.c
+++ b/drivers/net/wireless/ath/ath10k/qmi.c
@@ -917,7 +917,7 @@ static void ath10k_qmi_msa_ready_ind(struct qmi_handle *qmi_hdl,
ath10k_qmi_driver_event_post(qmi, ATH10K_QMI_EVENT_MSA_READY_IND, NULL);
}
-static struct qmi_msg_handler qmi_msg_handler[] = {
+static const struct qmi_msg_handler qmi_msg_handler[] = {
{
.type = QMI_INDICATION,
.msg_id = QMI_WLFW_FW_READY_IND_V01,
@@ -981,7 +981,7 @@ static void ath10k_qmi_del_server(struct qmi_handle *qmi_hdl,
NULL);
}
-static struct qmi_ops ath10k_qmi_ops = {
+static const struct qmi_ops ath10k_qmi_ops = {
.new_server = ath10k_qmi_new_server,
.del_server = ath10k_qmi_del_server,
};
--
2.29.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox