* Re: [PATCH v2] usbnet: optimize usbnet_bh() to reduce CPU load
From: Greg KH @ 2022-12-21 6:32 UTC (permalink / raw)
To: Leesoo Ahn
Cc: Oliver Neukum, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, linux-usb, linux-kernel
In-Reply-To: <20221221044230.1012787-1-lsahn@ooseel.net>
On Wed, Dec 21, 2022 at 01:42:30PM +0900, Leesoo Ahn wrote:
> The current source pushes skb into dev->done queue by calling
> skb_queue_tail() and then pop it by calling skb_dequeue() to branch to
> rx_cleanup state for freeing urb/skb in usbnet_bh(). It takes extra CPU
> load, 2.21% (skb_queue_tail) as follows.
>
> - 11.58% 0.26% swapper [k] usbnet_bh
> - 11.32% usbnet_bh
> - 6.43% skb_dequeue
> 6.34% _raw_spin_unlock_irqrestore
> - 2.21% skb_queue_tail
> 2.19% _raw_spin_unlock_irqrestore
> - 1.68% consume_skb
> - 0.97% kfree_skbmem
> 0.80% kmem_cache_free
> 0.53% skb_release_data
>
> To reduce the extra CPU load use return values jumping to rx_cleanup
> state directly to free them instead of calling skb_queue_tail() and
> skb_dequeue() for push/pop respectively.
>
> - 7.87% 0.25% swapper [k] usbnet_bh
> - 7.62% usbnet_bh
> - 4.81% skb_dequeue
> 4.74% _raw_spin_unlock_irqrestore
> - 1.75% consume_skb
> - 0.98% kfree_skbmem
> 0.78% kmem_cache_free
> 0.58% skb_release_data
> 0.53% smsc95xx_rx_fixup
>
> Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
> ---
> v2:
> - Replace goto label with return statement to reduce goto entropy
> - Add CPU load information by perf in commit message
>
> v1 at:
> https://patchwork.kernel.org/project/netdevbpf/patch/20221217161851.829497-1-lsahn@ooseel.net/
> ---
> drivers/net/usb/usbnet.c | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 64a9a80b2309..6e82fef90dd9 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -555,32 +555,30 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags)
>
> /*-------------------------------------------------------------------------*/
>
> -static inline void rx_process (struct usbnet *dev, struct sk_buff *skb)
> +static inline int rx_process(struct usbnet *dev, struct sk_buff *skb)
> {
> if (dev->driver_info->rx_fixup &&
> !dev->driver_info->rx_fixup (dev, skb)) {
> /* With RX_ASSEMBLE, rx_fixup() must update counters */
> if (!(dev->driver_info->flags & FLAG_RX_ASSEMBLE))
> dev->net->stats.rx_errors++;
> - goto done;
> + return 1;
"1" means that you processed 1 byte, not that this is an error, which is
what you want to say here, right? Please return a negative error value
like I asked this to be changed to last time :(
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] vhost_vdpa: fix the compile issue in commit 881ac7d2314f
From: Michael S. Tsirkin @ 2022-12-21 6:35 UTC (permalink / raw)
To: Jason Wang; +Cc: Cindy Lu, kvm, linux-kernel, virtualization, netdev, stable
In-Reply-To: <CACGkMEuJuUrA220XgHDOruK-aHWSfJ6mTaqNVQCAcOsPEwV91A@mail.gmail.com>
On Wed, Dec 21, 2022 at 11:23:09AM +0800, Jason Wang wrote:
> On Tue, Dec 20, 2022 at 10:02 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > The input of vhost_vdpa_iotlb_unmap() was changed in 881ac7d2314f,
> > But some function was not changed while calling this function.
> > Add this change
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 881ac7d2314f ("vhost_vdpa: fix the crash in unmap a large memory")
>
> Is this commit merged into Linus tree?
>
> Btw, Michael, I'd expect there's a respin of the patch so maybe Cindy
> can squash the fix into the new version?
>
> Thanks
Thanks, I fixed it myself already. Why do you want a respin?
That will mean trouble as the fixed patch is now being tested.
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> > drivers/vhost/vdpa.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > index 46ce35bea705..ec32f785dfde 100644
> > --- a/drivers/vhost/vdpa.c
> > +++ b/drivers/vhost/vdpa.c
> > @@ -66,8 +66,8 @@ static DEFINE_IDA(vhost_vdpa_ida);
> > static dev_t vhost_vdpa_major;
> >
> > static void vhost_vdpa_iotlb_unmap(struct vhost_vdpa *v,
> > - struct vhost_iotlb *iotlb,
> > - u64 start, u64 last);
> > + struct vhost_iotlb *iotlb, u64 start,
> > + u64 last, u32 asid);
> >
> > static inline u32 iotlb_to_asid(struct vhost_iotlb *iotlb)
> > {
> > @@ -139,7 +139,7 @@ static int vhost_vdpa_remove_as(struct vhost_vdpa *v, u32 asid)
> > return -EINVAL;
> >
> > hlist_del(&as->hash_link);
> > - vhost_vdpa_iotlb_unmap(v, &as->iotlb, 0ULL, 0ULL - 1);
> > + vhost_vdpa_iotlb_unmap(v, &as->iotlb, 0ULL, 0ULL - 1, asid);
> > kfree(as);
> >
> > return 0;
> > --
> > 2.34.3
> >
^ permalink raw reply
* Re: [PATCH] vhost_vdpa: fix the compile issue in commit 881ac7d2314f
From: Michael S. Tsirkin @ 2022-12-21 6:36 UTC (permalink / raw)
To: Cindy Lu; +Cc: Jason Wang, kvm, linux-kernel, virtualization, netdev, stable
In-Reply-To: <CACLfguUgsWrE+ZFxJYd-h81AvMQFio0-VU9oE0kpj7t5D2pJvg@mail.gmail.com>
On Wed, Dec 21, 2022 at 01:58:11PM +0800, Cindy Lu wrote:
> On Wed, 21 Dec 2022 at 11:23, Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Tue, Dec 20, 2022 at 10:02 PM Cindy Lu <lulu@redhat.com> wrote:
> > >
> > > The input of vhost_vdpa_iotlb_unmap() was changed in 881ac7d2314f,
> > > But some function was not changed while calling this function.
> > > Add this change
> > >
> > > Cc: stable@vger.kernel.org
> > > Fixes: 881ac7d2314f ("vhost_vdpa: fix the crash in unmap a large memory")
> >
> > Is this commit merged into Linus tree?
> >
> > Btw, Michael, I'd expect there's a respin of the patch so maybe Cindy
> > can squash the fix into the new version?
> >
> > Thanks
> >
> This is not merged in linus tree, and this compile issue was hit in mst's tree
> should I send a new version squash the patch and the fix?
>
> Thanks
> Cindy
No I fixed it myself. Pls check my tree now if it's not ok let me know.
Thanks!
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > ---
> > > drivers/vhost/vdpa.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > > index 46ce35bea705..ec32f785dfde 100644
> > > --- a/drivers/vhost/vdpa.c
> > > +++ b/drivers/vhost/vdpa.c
> > > @@ -66,8 +66,8 @@ static DEFINE_IDA(vhost_vdpa_ida);
> > > static dev_t vhost_vdpa_major;
> > >
> > > static void vhost_vdpa_iotlb_unmap(struct vhost_vdpa *v,
> > > - struct vhost_iotlb *iotlb,
> > > - u64 start, u64 last);
> > > + struct vhost_iotlb *iotlb, u64 start,
> > > + u64 last, u32 asid);
> > >
> > > static inline u32 iotlb_to_asid(struct vhost_iotlb *iotlb)
> > > {
> > > @@ -139,7 +139,7 @@ static int vhost_vdpa_remove_as(struct vhost_vdpa *v, u32 asid)
> > > return -EINVAL;
> > >
> > > hlist_del(&as->hash_link);
> > > - vhost_vdpa_iotlb_unmap(v, &as->iotlb, 0ULL, 0ULL - 1);
> > > + vhost_vdpa_iotlb_unmap(v, &as->iotlb, 0ULL, 0ULL - 1, asid);
> > > kfree(as);
> > >
> > > return 0;
> > > --
> > > 2.34.3
> > >
> >
^ permalink raw reply
* Re: [PATCH] vhost_vdpa: fix the compile issue in commit 881ac7d2314f
From: Jason Wang @ 2022-12-21 6:48 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Cindy Lu, kvm, linux-kernel, virtualization, netdev, stable
In-Reply-To: <20221221013359-mutt-send-email-mst@kernel.org>
On Wed, Dec 21, 2022 at 2:35 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Dec 21, 2022 at 11:23:09AM +0800, Jason Wang wrote:
> > On Tue, Dec 20, 2022 at 10:02 PM Cindy Lu <lulu@redhat.com> wrote:
> > >
> > > The input of vhost_vdpa_iotlb_unmap() was changed in 881ac7d2314f,
> > > But some function was not changed while calling this function.
> > > Add this change
> > >
> > > Cc: stable@vger.kernel.org
> > > Fixes: 881ac7d2314f ("vhost_vdpa: fix the crash in unmap a large memory")
> >
> > Is this commit merged into Linus tree?
> >
> > Btw, Michael, I'd expect there's a respin of the patch so maybe Cindy
> > can squash the fix into the new version?
> >
> > Thanks
>
> Thanks, I fixed it myself already. Why do you want a respin?
For some reason I miss v4, so it should be fine.
Thanks
> That will mean trouble as the fixed patch is now being tested.
>
>
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > ---
> > > drivers/vhost/vdpa.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > > index 46ce35bea705..ec32f785dfde 100644
> > > --- a/drivers/vhost/vdpa.c
> > > +++ b/drivers/vhost/vdpa.c
> > > @@ -66,8 +66,8 @@ static DEFINE_IDA(vhost_vdpa_ida);
> > > static dev_t vhost_vdpa_major;
> > >
> > > static void vhost_vdpa_iotlb_unmap(struct vhost_vdpa *v,
> > > - struct vhost_iotlb *iotlb,
> > > - u64 start, u64 last);
> > > + struct vhost_iotlb *iotlb, u64 start,
> > > + u64 last, u32 asid);
> > >
> > > static inline u32 iotlb_to_asid(struct vhost_iotlb *iotlb)
> > > {
> > > @@ -139,7 +139,7 @@ static int vhost_vdpa_remove_as(struct vhost_vdpa *v, u32 asid)
> > > return -EINVAL;
> > >
> > > hlist_del(&as->hash_link);
> > > - vhost_vdpa_iotlb_unmap(v, &as->iotlb, 0ULL, 0ULL - 1);
> > > + vhost_vdpa_iotlb_unmap(v, &as->iotlb, 0ULL, 0ULL - 1, asid);
> > > kfree(as);
> > >
> > > return 0;
> > > --
> > > 2.34.3
> > >
>
^ permalink raw reply
* [PATCH] ixp4xx_eth: Fix an error handling path in ixp4xx_eth_probe()
From: Christophe JAILLET @ 2022-12-21 7:17 UTC (permalink / raw)
To: Krzysztof Halasa, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Krzysztof Hałasa
Cc: linux-kernel, kernel-janitors, Christophe JAILLET, netdev
If an error occurs after a successful ixp4xx_mdio_register() call, it
should be undone by a corresponding ixp4xx_mdio_remove().
Add the missing call in the error handling path, as already done in the
remove function.
Fixes: 2098c18d6cf6 ("IXP4xx: Add PHYLIB support to Ethernet driver.")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
drivers/net/ethernet/xscale/ixp4xx_eth.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/xscale/ixp4xx_eth.c b/drivers/net/ethernet/xscale/ixp4xx_eth.c
index 3b0c5f177447..007d68b385a5 100644
--- a/drivers/net/ethernet/xscale/ixp4xx_eth.c
+++ b/drivers/net/ethernet/xscale/ixp4xx_eth.c
@@ -1490,8 +1490,10 @@ static int ixp4xx_eth_probe(struct platform_device *pdev)
netif_napi_add_weight(ndev, &port->napi, eth_poll, NAPI_WEIGHT);
- if (!(port->npe = npe_request(NPE_ID(port->id))))
- return -EIO;
+ if (!(port->npe = npe_request(NPE_ID(port->id)))) {
+ err = -EIO;
+ goto err_remove_mdio;
+ }
port->plat = plat;
npe_port_tab[NPE_ID(port->id)] = port;
@@ -1530,6 +1532,8 @@ static int ixp4xx_eth_probe(struct platform_device *pdev)
err_free_mem:
npe_port_tab[NPE_ID(port->id)] = NULL;
npe_release(port->npe);
+err_remove_mdio:
+ ixp4xx_mdio_remove();
return err;
}
--
2.34.1
^ permalink raw reply related
* Re: [RFC PATCH 6/6] vdpa_sim: add support for user VA
From: Eugenio Perez Martin @ 2022-12-21 7:17 UTC (permalink / raw)
To: Stefano Garzarella
Cc: virtualization, Jason Wang, Andrey Zhadchenko, linux-kernel, kvm,
Michael S. Tsirkin, stefanha, netdev
In-Reply-To: <20221214163025.103075-7-sgarzare@redhat.com>
On Wed, Dec 14, 2022 at 5:31 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> The new "use_va" module parameter (default: false) is used in
Why not true by default? I'd say it makes more sense for the simulator
to use va mode and only use pa for testing it.
> vdpa_alloc_device() to inform the vDPA framework that the device
> supports VA.
>
> vringh is initialized to use VA only when "use_va" is true and the
> user's mm has been bound. So, only when the bus supports user VA
> (e.g. vhost-vdpa).
>
> vdpasim_mm_work_fn work is used to attach the kthread to the user
> address space when the .bind_mm callback is invoked, and to detach
> it when the device is reset.
>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> drivers/vdpa/vdpa_sim/vdpa_sim.h | 1 +
> drivers/vdpa/vdpa_sim/vdpa_sim.c | 104 ++++++++++++++++++++++++++++++-
> 2 files changed, 103 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> index 07ef53ea375e..1b010e5c0445 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> @@ -55,6 +55,7 @@ struct vdpasim {
> struct vdpasim_virtqueue *vqs;
> struct kthread_worker *worker;
> struct kthread_work work;
> + struct mm_struct *mm_bound;
> struct vdpasim_dev_attr dev_attr;
> /* spinlock to synchronize virtqueue state */
> spinlock_t lock;
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> index 36a1d2e0a6ba..6e07cedef30c 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> @@ -36,10 +36,90 @@ module_param(max_iotlb_entries, int, 0444);
> MODULE_PARM_DESC(max_iotlb_entries,
> "Maximum number of iotlb entries for each address space. 0 means unlimited. (default: 2048)");
>
> +static bool use_va;
> +module_param(use_va, bool, 0444);
> +MODULE_PARM_DESC(use_va, "Enable the device's ability to use VA");
> +
> #define VDPASIM_QUEUE_ALIGN PAGE_SIZE
> #define VDPASIM_QUEUE_MAX 256
> #define VDPASIM_VENDOR_ID 0
>
> +struct vdpasim_mm_work {
> + struct kthread_work work;
> + struct task_struct *owner;
> + struct mm_struct *mm;
> + bool bind;
> + int ret;
> +};
> +
> +static void vdpasim_mm_work_fn(struct kthread_work *work)
> +{
> + struct vdpasim_mm_work *mm_work =
> + container_of(work, struct vdpasim_mm_work, work);
> +
> + mm_work->ret = 0;
> +
> + if (mm_work->bind) {
> + kthread_use_mm(mm_work->mm);
> +#if 0
> + if (mm_work->owner)
> + mm_work->ret = cgroup_attach_task_all(mm_work->owner,
> + current);
> +#endif
> + } else {
> +#if 0
> + //TODO: check it
> + cgroup_release(current);
> +#endif
> + kthread_unuse_mm(mm_work->mm);
> + }
> +}
> +
> +static void vdpasim_worker_queue_mm(struct vdpasim *vdpasim,
> + struct vdpasim_mm_work *mm_work)
> +{
> + struct kthread_work *work = &mm_work->work;
> +
> + kthread_init_work(work, vdpasim_mm_work_fn);
> + kthread_queue_work(vdpasim->worker, work);
> +
> + spin_unlock(&vdpasim->lock);
> + kthread_flush_work(work);
> + spin_lock(&vdpasim->lock);
> +}
> +
> +static int vdpasim_worker_bind_mm(struct vdpasim *vdpasim,
> + struct mm_struct *new_mm,
> + struct task_struct *owner)
> +{
> + struct vdpasim_mm_work mm_work;
> +
> + mm_work.owner = owner;
> + mm_work.mm = new_mm;
> + mm_work.bind = true;
> +
> + vdpasim_worker_queue_mm(vdpasim, &mm_work);
> +
> + if (!mm_work.ret)
> + vdpasim->mm_bound = new_mm;
> +
> + return mm_work.ret;
> +}
> +
> +static void vdpasim_worker_unbind_mm(struct vdpasim *vdpasim)
> +{
> + struct vdpasim_mm_work mm_work;
> +
> + if (!vdpasim->mm_bound)
> + return;
> +
> + mm_work.mm = vdpasim->mm_bound;
> + mm_work.bind = false;
> +
> + vdpasim_worker_queue_mm(vdpasim, &mm_work);
> +
> + vdpasim->mm_bound = NULL;
> +}
> static struct vdpasim *vdpa_to_sim(struct vdpa_device *vdpa)
> {
> return container_of(vdpa, struct vdpasim, vdpa);
> @@ -66,8 +146,10 @@ static void vdpasim_vq_notify(struct vringh *vring)
> static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx)
> {
> struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx];
> + bool va_enabled = use_va && vdpasim->mm_bound;
>
> - vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, false, false,
> + vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, false,
> + va_enabled,
> (struct vring_desc *)(uintptr_t)vq->desc_addr,
> (struct vring_avail *)
> (uintptr_t)vq->driver_addr,
> @@ -96,6 +178,9 @@ static void vdpasim_do_reset(struct vdpasim *vdpasim)
> {
> int i;
>
> + //TODO: should we cancel the works?
> + vdpasim_worker_unbind_mm(vdpasim);
> +
> spin_lock(&vdpasim->iommu_lock);
>
> for (i = 0; i < vdpasim->dev_attr.nvqs; i++) {
> @@ -275,7 +360,7 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *dev_attr,
>
> vdpasim = vdpa_alloc_device(struct vdpasim, vdpa, NULL, ops,
> dev_attr->ngroups, dev_attr->nas,
> - dev_attr->name, false);
> + dev_attr->name, use_va);
> if (IS_ERR(vdpasim)) {
> ret = PTR_ERR(vdpasim);
> goto err_alloc;
> @@ -657,6 +742,19 @@ static int vdpasim_set_map(struct vdpa_device *vdpa, unsigned int asid,
> return ret;
> }
>
> +static int vdpasim_bind_mm(struct vdpa_device *vdpa, struct mm_struct *mm,
> + struct task_struct *owner)
> +{
> + struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> + int ret;
> +
> + spin_lock(&vdpasim->lock);
> + ret = vdpasim_worker_bind_mm(vdpasim, mm, owner);
> + spin_unlock(&vdpasim->lock);
> +
> + return ret;
> +}
> +
> static int vdpasim_dma_map(struct vdpa_device *vdpa, unsigned int asid,
> u64 iova, u64 size,
> u64 pa, u32 perm, void *opaque)
> @@ -744,6 +842,7 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
> .set_group_asid = vdpasim_set_group_asid,
> .dma_map = vdpasim_dma_map,
> .dma_unmap = vdpasim_dma_unmap,
> + .bind_mm = vdpasim_bind_mm,
> .free = vdpasim_free,
> };
>
> @@ -776,6 +875,7 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = {
> .get_iova_range = vdpasim_get_iova_range,
> .set_group_asid = vdpasim_set_group_asid,
> .set_map = vdpasim_set_map,
> + .bind_mm = vdpasim_bind_mm,
> .free = vdpasim_free,
> };
>
> --
> 2.38.1
>
^ permalink raw reply
* Re: [PATCH v2] usbnet: optimize usbnet_bh() to reduce CPU load
From: Leesoo Ahn @ 2022-12-21 7:19 UTC (permalink / raw)
To: Greg KH
Cc: Oliver Neukum, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, linux-usb, linux-kernel
In-Reply-To: <Y6KoglOyuFEqfp2k@kroah.com>
On 22. 12. 21. 15:32, Greg KH wrote:
> On Wed, Dec 21, 2022 at 01:42:30PM +0900, Leesoo Ahn wrote:
>> The current source pushes skb into dev->done queue by calling
>> skb_queue_tail() and then pop it by calling skb_dequeue() to branch to
>> rx_cleanup state for freeing urb/skb in usbnet_bh(). It takes extra CPU
>> load, 2.21% (skb_queue_tail) as follows.
>>
>> - 11.58% 0.26% swapper [k] usbnet_bh
>> - 11.32% usbnet_bh
>> - 6.43% skb_dequeue
>> 6.34% _raw_spin_unlock_irqrestore
>> - 2.21% skb_queue_tail
>> 2.19% _raw_spin_unlock_irqrestore
>> - 1.68% consume_skb
>> - 0.97% kfree_skbmem
>> 0.80% kmem_cache_free
>> 0.53% skb_release_data
>>
>> To reduce the extra CPU load use return values jumping to rx_cleanup
>> state directly to free them instead of calling skb_queue_tail() and
>> skb_dequeue() for push/pop respectively.
>>
>> - 7.87% 0.25% swapper [k] usbnet_bh
>> - 7.62% usbnet_bh
>> - 4.81% skb_dequeue
>> 4.74% _raw_spin_unlock_irqrestore
>> - 1.75% consume_skb
>> - 0.98% kfree_skbmem
>> 0.78% kmem_cache_free
>> 0.58% skb_release_data
>> 0.53% smsc95xx_rx_fixup
>>
>> Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
>> ---
>> v2:
>> - Replace goto label with return statement to reduce goto entropy
>> - Add CPU load information by perf in commit message
>>
>> v1 at:
>> https://patchwork.kernel.org/project/netdevbpf/patch/20221217161851.829497-1-lsahn@ooseel.net/
>> ---
>> drivers/net/usb/usbnet.c | 19 +++++++++----------
>> 1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>> index 64a9a80b2309..6e82fef90dd9 100644
>> --- a/drivers/net/usb/usbnet.c
>> +++ b/drivers/net/usb/usbnet.c
>> @@ -555,32 +555,30 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags)
>>
>> /*-------------------------------------------------------------------------*/
>>
>> -static inline void rx_process (struct usbnet *dev, struct sk_buff *skb)
>> +static inline int rx_process(struct usbnet *dev, struct sk_buff *skb)
>> {
>> if (dev->driver_info->rx_fixup &&
>> !dev->driver_info->rx_fixup (dev, skb)) {
>> /* With RX_ASSEMBLE, rx_fixup() must update counters */
>> if (!(dev->driver_info->flags & FLAG_RX_ASSEMBLE))
>> dev->net->stats.rx_errors++;
>> - goto done;
>> + return 1;
> "1" means that you processed 1 byte, not that this is an error, which is
> what you want to say here, right?
No not at all..
> Please return a negative error value
> like I asked this to be changed to last time :(
Could you help me to decide the message type at this point please? I am
confused.
The return value totally depends on how rx_fixup() is. For instance, in
smsc95xx.c, smsc95xx_rx_fixup() function returns 0 in two cases that
1) frame size is greater than ETH_FRAME_LEN(1526 bytes) as follows
1853 /* ETH_FRAME_LEN + 4(CRC) + 2(COE) + 4(Vlan) */
1854 if (unlikely(size > (ETH_FRAME_LEN + 12))) {
1855 netif_dbg(dev, rx_err, dev->net,
1856 "size err header=0x%08x\n", header);
1857 return 0;
1858 }
2) it is failed for skb allocation, but memory?
1870 ax_skb = skb_clone(skb, GFP_ATOMIC);
1871 if (unlikely(!ax_skb)) {
1872 netdev_warn(dev->net, "Error allocating skb\n");
1873 return 0;
1874 }
I guess EPROTO or ENOMEM, one of them could be the value at the point
but I have no ideas..
Best regards,
Leesoo
^ permalink raw reply
* Re: kernel BUG in __skb_gso_segment
From: Tudor Ambarus @ 2022-12-21 7:28 UTC (permalink / raw)
To: Willem de Bruijn, gregkh
Cc: mst, jasowang, virtualization, edumazet, davem, kuba, pabeni,
netdev, willemb, syzkaller, liuhangbin, linux-kernel, joneslee
In-Reply-To: <CAF=yD-KEwVnH6PRyxbJZt4iGfKasadYwU_6_V+hHW2s+ZqFNcw@mail.gmail.com>
Hi,
I added Greg KH to the thread, maybe he can shed some light on whether
new support can be marked as fixes and backported to stable. The rules
on what kind of patches are accepted into the -stable tree don't mention
new support:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
On 20.12.2022 20:27, Willem de Bruijn wrote:
> On Tue, Dec 20, 2022 at 8:21 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> Hi,
>>
>> There's a bug [1] reported by syzkaller in linux-5.15.y that I'd like
>> to squash. The commit in stable that introduces the bug is:
>> b99c71f90978 net: skip virtio_net_hdr_set_proto if protocol already set
>> The upstream commit for this is:
>> 1ed1d592113959f00cc552c3b9f47ca2d157768f
>>
>> I discovered that in mainline this bug was squashed by the following
>> commits:
>> e9d3f80935b6 ("net/af_packet: make sure to pull mac header")
>> dfed913e8b55 ("net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO")
>>
>> I'm seeking for some guidance on how to fix linux-5.15.y. From what I
>> understand, the bug in stable is triggered because we end up with a
>> header offset of 18, that eventually triggers the GSO crash in
>> __skb_pull. If I revert the commit in culprit from linux-5.15.y, we'll
>> end up with a header offset of 14, the bug is not hit and the packet is
>> dropped at validate_xmit_skb() time. I'm wondering if reverting it is
>> the right thing to do, as the commit is marked as a fix. Backporting the
>> 2 commits from mainline is not an option as they introduce new support.
>> Would such a patch be better than reverting the offending commit?
>
> If both patches can be backported without conflicts, in this case I
> think that is the preferred solution.
I confirm both patches can be backported without conflicts.
>
> If the fix were obvious that would be an option. But the history for
> this code indicates that it isn't. It has a history of fixes for edge
> cases.
>
> Backporting the two avoids a fork that would make backporting
> additional fixes harder. The first of the two is technically not a
I agree that a fork would make backporting additional fixes harder.
I'm no networking guy, but I can debug further to understand whether the
patch that I proposed or other would make sense for both mainline and
stable kernels. We'll avoid the fork this way.
> fix, but evidently together they are for this case. And the additional
> logic and risk backported seems manageable.
It is, indeed.
>
> Admittedly that is subjective. I can help take a closer look at a
> custom fix if consensus is that is preferable.
Thanks. Let's wait for others to comment so that we have an agreement.
Cheers,
ta
^ permalink raw reply
* Re: [PATCH v2] usbnet: optimize usbnet_bh() to reduce CPU load
From: Greg KH @ 2022-12-21 7:30 UTC (permalink / raw)
To: Leesoo Ahn
Cc: Oliver Neukum, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, linux-usb, linux-kernel
In-Reply-To: <2d4033ea-3034-24cf-493c-f60258f9988d@ooseel.net>
On Wed, Dec 21, 2022 at 04:19:45PM +0900, Leesoo Ahn wrote:
>
> On 22. 12. 21. 15:32, Greg KH wrote:
> > On Wed, Dec 21, 2022 at 01:42:30PM +0900, Leesoo Ahn wrote:
> > > The current source pushes skb into dev->done queue by calling
> > > skb_queue_tail() and then pop it by calling skb_dequeue() to branch to
> > > rx_cleanup state for freeing urb/skb in usbnet_bh(). It takes extra CPU
> > > load, 2.21% (skb_queue_tail) as follows.
> > >
> > > - 11.58% 0.26% swapper [k] usbnet_bh
> > > - 11.32% usbnet_bh
> > > - 6.43% skb_dequeue
> > > 6.34% _raw_spin_unlock_irqrestore
> > > - 2.21% skb_queue_tail
> > > 2.19% _raw_spin_unlock_irqrestore
> > > - 1.68% consume_skb
> > > - 0.97% kfree_skbmem
> > > 0.80% kmem_cache_free
> > > 0.53% skb_release_data
> > >
> > > To reduce the extra CPU load use return values jumping to rx_cleanup
> > > state directly to free them instead of calling skb_queue_tail() and
> > > skb_dequeue() for push/pop respectively.
> > >
> > > - 7.87% 0.25% swapper [k] usbnet_bh
> > > - 7.62% usbnet_bh
> > > - 4.81% skb_dequeue
> > > 4.74% _raw_spin_unlock_irqrestore
> > > - 1.75% consume_skb
> > > - 0.98% kfree_skbmem
> > > 0.78% kmem_cache_free
> > > 0.58% skb_release_data
> > > 0.53% smsc95xx_rx_fixup
> > >
> > > Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
> > > ---
> > > v2:
> > > - Replace goto label with return statement to reduce goto entropy
> > > - Add CPU load information by perf in commit message
> > >
> > > v1 at:
> > > https://patchwork.kernel.org/project/netdevbpf/patch/20221217161851.829497-1-lsahn@ooseel.net/
> > > ---
> > > drivers/net/usb/usbnet.c | 19 +++++++++----------
> > > 1 file changed, 9 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> > > index 64a9a80b2309..6e82fef90dd9 100644
> > > --- a/drivers/net/usb/usbnet.c
> > > +++ b/drivers/net/usb/usbnet.c
> > > @@ -555,32 +555,30 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags)
> > > /*-------------------------------------------------------------------------*/
> > > -static inline void rx_process (struct usbnet *dev, struct sk_buff *skb)
> > > +static inline int rx_process(struct usbnet *dev, struct sk_buff *skb)
> > > {
> > > if (dev->driver_info->rx_fixup &&
> > > !dev->driver_info->rx_fixup (dev, skb)) {
> > > /* With RX_ASSEMBLE, rx_fixup() must update counters */
> > > if (!(dev->driver_info->flags & FLAG_RX_ASSEMBLE))
> > > dev->net->stats.rx_errors++;
> > > - goto done;
> > > + return 1;
> > "1" means that you processed 1 byte, not that this is an error, which is
> > what you want to say here, right?
> No not at all..
> > Please return a negative error value
> > like I asked this to be changed to last time :(
> Could you help me to decide the message type at this point please? I am
> confused.
I do not know, pick something that seems correct and we can go from
there. The important thing is that it is a -ERR value, not a positive
one as that makes no sense for kernel functions.
thanks,
greg k-h
^ permalink raw reply
* Re: kernel BUG in __skb_gso_segment
From: Greg KH @ 2022-12-21 7:37 UTC (permalink / raw)
To: Tudor Ambarus
Cc: Willem de Bruijn, mst, jasowang, virtualization, edumazet, davem,
kuba, pabeni, netdev, willemb, syzkaller, liuhangbin,
linux-kernel, joneslee
In-Reply-To: <a13f83f3-737d-1bfe-c9ef-031a6cd4d131@linaro.org>
On Wed, Dec 21, 2022 at 09:28:16AM +0200, Tudor Ambarus wrote:
> Hi,
>
> I added Greg KH to the thread, maybe he can shed some light on whether
> new support can be marked as fixes and backported to stable. The rules
> on what kind of patches are accepted into the -stable tree don't mention
> new support:
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
As you say, we don't take new features into older kernels. Unless they
fix a reported problem, if so, submit the git ids to us and we will be
glad to review them.
thanks,
greg k-h
^ permalink raw reply
* Re: kernel BUG in __skb_gso_segment
From: Tudor Ambarus @ 2022-12-21 7:42 UTC (permalink / raw)
To: Greg KH
Cc: Willem de Bruijn, mst, jasowang, virtualization, edumazet, davem,
kuba, pabeni, netdev, willemb, syzkaller, liuhangbin,
linux-kernel, joneslee
In-Reply-To: <Y6K3q6Bo3wwC57bK@kroah.com>
On 21.12.2022 09:37, Greg KH wrote:
> On Wed, Dec 21, 2022 at 09:28:16AM +0200, Tudor Ambarus wrote:
>> Hi,
>>
>> I added Greg KH to the thread, maybe he can shed some light on whether
>> new support can be marked as fixes and backported to stable. The rules
>> on what kind of patches are accepted into the -stable tree don't mention
>> new support:
>> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>
> As you say, we don't take new features into older kernels. Unless they
> fix a reported problem, if so, submit the git ids to us and we will be
> glad to review them.
>
They do fix a bug. I'm taking care of it. Shall I update
Documentation/process/stable-kernel-rules.rst to mention this rule as
well?
Thanks,
ta
^ permalink raw reply
* Re: [PATCH v1] qlcnic: prevent ->dcb use-after-free on qlcnic_dcb_enable() failure
From: Daniil Tatianin @ 2022-12-21 7:47 UTC (permalink / raw)
To: Michal Swiatkowski
Cc: Shahed Shaikh, Manish Chopra, GR-Linux-NIC-Dev, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel
In-Reply-To: <Y6G5eWWucdaJXmQu@localhost.localdomain>
On 12/20/22 4:32 PM, Michal Swiatkowski wrote:
> On Tue, Dec 20, 2022 at 03:56:49PM +0300, Daniil Tatianin wrote:
>> adapter->dcb would get silently freed inside qlcnic_dcb_enable() in
>> case qlcnic_dcb_attach() would return an error, which always happens
>> under OOM conditions. This would lead to use-after-free because both
>> of the existing callers invoke qlcnic_dcb_get_info() on the obtained
>> pointer, which is potentially freed at that point.
>>
>> Propagate errors from qlcnic_dcb_enable(), and instead free the dcb
>> pointer at callsite.
>>
>> Found by Linux Verification Center (linuxtesting.org) with the SVACE
>> static analysis tool.
>>
>> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
>
> Please add Fix tag and net as target (net-next is close till the end of
> this year)
>
Sorry, I'll include a fixes tag.
Could you please explain what I would have to do to add net as target?
>> ---
>> drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c | 9 ++++++++-
>> drivers/net/ethernet/qlogic/qlcnic/qlcnic_dcb.h | 5 ++---
>> drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 9 ++++++++-
>> 3 files changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c
>> index dbb800769cb6..465f149d94d4 100644
>> --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c
>> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c
>> @@ -2505,7 +2505,14 @@ int qlcnic_83xx_init(struct qlcnic_adapter *adapter)
>> goto disable_mbx_intr;
>>
>> qlcnic_83xx_clear_function_resources(adapter);
>> - qlcnic_dcb_enable(adapter->dcb);
>> +
>> + err = qlcnic_dcb_enable(adapter->dcb);
>> + if (err) {
>> + qlcnic_clear_dcb_ops(adapter->dcb);
>> + adapter->dcb = NULL;
>> + goto disable_mbx_intr;
>> + }
>
> Maybe I miss sth but it looks like there can be memory leak.
> For example if error in attach happen after allocating of dcb->cfg.
> Isn't it better to call qlcnic_dcb_free instead of qlcnic_clear_dcb_ops?
I think you're right, if attach fails midway then we might leak cfg or
something else.
I'll use qlcnic_dcb_free() instead and completely remove
qlcnic_clear_dcb_ops() as it
seems to be unused and relies on dcb being in a very specific state.
>> +
>> qlcnic_83xx_initialize_nic(adapter, 1);
>> qlcnic_dcb_get_info(adapter->dcb);
>>
>> diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_dcb.h b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_dcb.h
>> index 7519773eaca6..e1460f9c38bf 100644
>> --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_dcb.h
>> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_dcb.h
>> @@ -112,9 +112,8 @@ static inline void qlcnic_dcb_init_dcbnl_ops(struct qlcnic_dcb *dcb)
>> dcb->ops->init_dcbnl_ops(dcb);
>> }
>>
>> -static inline void qlcnic_dcb_enable(struct qlcnic_dcb *dcb)
>> +static inline int qlcnic_dcb_enable(struct qlcnic_dcb *dcb)
>> {
>> - if (dcb && qlcnic_dcb_attach(dcb))
>> - qlcnic_clear_dcb_ops(dcb);
>> + return dcb ? qlcnic_dcb_attach(dcb) : 0;
>> }
>> #endif
>> diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
>> index 28476b982bab..36ba15fc9776 100644
>> --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
>> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
>> @@ -2599,7 +2599,14 @@ qlcnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> "Device does not support MSI interrupts\n");
>>
>> if (qlcnic_82xx_check(adapter)) {
>> - qlcnic_dcb_enable(adapter->dcb);
>> + err = qlcnic_dcb_enable(adapter->dcb);
>> + if (err) {
>> + qlcnic_clear_dcb_ops(adapter->dcb);
>> + adapter->dcb = NULL;
>> + dev_err(&pdev->dev, "Failed to enable DCB\n");
>> + goto err_out_free_hw;
>> + }
>> +
>> qlcnic_dcb_get_info(adapter->dcb);
>> err = qlcnic_setup_intr(adapter);
>>
>> --
>> 2.25.1
>>
^ permalink raw reply
* Re: [PATCH v2] usbnet: optimize usbnet_bh() to reduce CPU load
From: Leesoo Ahn @ 2022-12-21 7:50 UTC (permalink / raw)
To: Greg KH
Cc: Oliver Neukum, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, linux-usb, linux-kernel
In-Reply-To: <Y6K2L+t5NjK/3ipj@kroah.com>
On 22. 12. 21. 16:30, Greg KH wrote:
> On Wed, Dec 21, 2022 at 04:19:45PM +0900, Leesoo Ahn wrote:
>> On 22. 12. 21. 15:32, Greg KH wrote:
>>> On Wed, Dec 21, 2022 at 01:42:30PM +0900, Leesoo Ahn wrote:
>>>> The current source pushes skb into dev->done queue by calling
>>>> skb_queue_tail() and then pop it by calling skb_dequeue() to branch to
>>>> rx_cleanup state for freeing urb/skb in usbnet_bh(). It takes extra CPU
>>>> load, 2.21% (skb_queue_tail) as follows.
>>>>
>>>> - 11.58% 0.26% swapper [k] usbnet_bh
>>>> - 11.32% usbnet_bh
>>>> - 6.43% skb_dequeue
>>>> 6.34% _raw_spin_unlock_irqrestore
>>>> - 2.21% skb_queue_tail
>>>> 2.19% _raw_spin_unlock_irqrestore
>>>> - 1.68% consume_skb
>>>> - 0.97% kfree_skbmem
>>>> 0.80% kmem_cache_free
>>>> 0.53% skb_release_data
>>>>
>>>> To reduce the extra CPU load use return values jumping to rx_cleanup
>>>> state directly to free them instead of calling skb_queue_tail() and
>>>> skb_dequeue() for push/pop respectively.
>>>>
>>>> - 7.87% 0.25% swapper [k] usbnet_bh
>>>> - 7.62% usbnet_bh
>>>> - 4.81% skb_dequeue
>>>> 4.74% _raw_spin_unlock_irqrestore
>>>> - 1.75% consume_skb
>>>> - 0.98% kfree_skbmem
>>>> 0.78% kmem_cache_free
>>>> 0.58% skb_release_data
>>>> 0.53% smsc95xx_rx_fixup
>>>>
>>>> Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
>>>> ---
>>>> v2:
>>>> - Replace goto label with return statement to reduce goto entropy
>>>> - Add CPU load information by perf in commit message
>>>>
>>>> v1 at:
>>>> https://patchwork.kernel.org/project/netdevbpf/patch/20221217161851.829497-1-lsahn@ooseel.net/
>>>> ---
>>>> drivers/net/usb/usbnet.c | 19 +++++++++----------
>>>> 1 file changed, 9 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>>>> index 64a9a80b2309..6e82fef90dd9 100644
>>>> --- a/drivers/net/usb/usbnet.c
>>>> +++ b/drivers/net/usb/usbnet.c
>>>> @@ -555,32 +555,30 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags)
>>>> /*-------------------------------------------------------------------------*/
>>>> -static inline void rx_process (struct usbnet *dev, struct sk_buff *skb)
>>>> +static inline int rx_process(struct usbnet *dev, struct sk_buff *skb)
>>>> {
>>>> if (dev->driver_info->rx_fixup &&
>>>> !dev->driver_info->rx_fixup (dev, skb)) {
>>>> /* With RX_ASSEMBLE, rx_fixup() must update counters */
>>>> if (!(dev->driver_info->flags & FLAG_RX_ASSEMBLE))
>>>> dev->net->stats.rx_errors++;
>>>> - goto done;
>>>> + return 1;
>>> "1" means that you processed 1 byte, not that this is an error, which is
>>> what you want to say here, right?
>> No not at all..
>>> Please return a negative error value
>>> like I asked this to be changed to last time :(
>> Could you help me to decide the message type at this point please? I am
>> confused.
> I do not know, pick something that seems correct and we can go from
> there. The important thing is that it is a -ERR value, not a positive
> one as that makes no sense for kernel functions.
Thank you for reviewing, v3 will be sent soon.
Best regards,
Leesoo
^ permalink raw reply
* Re: [PATCH net-next v2] net: phy: Add driver for Motorcomm yt8531 gigabit ethernet phy
From: Frank @ 2022-12-21 7:53 UTC (permalink / raw)
To: andrew
Cc: Frank.Sae, davem, edumazet, fei.zhang, hkallweit1, hua.sun, kuba,
linux-kernel, linux, netdev, pabeni, pgwipeout, xiaogang.fan
Hi Andrew,
I send you this mail on 2022-12-14 11:07 UTC, but it failed:
"Connect to the recipient Email system failed. (9)connect to the "vps0.lunn.ch(x.x.x.x) of lunn.ch" server failed"
so I send it again.
on Dec. 3, 2022, 8:47 p.m. UTC , Andrew Lunn wrote:
> On Fri, Dec 02, 2022 at 01:34:16PM +0000, Russell King (Oracle) wrote:
> > On Fri, Dec 02, 2022 at 02:27:43PM +0100, Andrew Lunn wrote:
> > > > +static bool mdio_is_locked(struct phy_device *phydev)
> > > > +{
> > > > + return mutex_is_locked(&phydev->mdio.bus->mdio_lock);
> > > > +}
> > > > +
> > > > +#define ASSERT_MDIO(phydev) \
> > > > + WARN_ONCE(!mdio_is_locked(phydev), \
> > > > + "MDIO: assertion failed at %s (%d)\n", __FILE__, __LINE__)
> > > > +
> > >
> > > Hi Frank
> > >
> > > You are not the only one who gets locking wrong. This could be used in
> > > other drivers. Please add it to include/linux/phy.h,
> >
> > That placement doesn't make much sense.
> >
> > As I already said, we have lockdep checks in drivers/net/phy/mdio_bus.c,
> > and if we want to increase their effectiveness, then that's the place
> > that it should be done.
>
> I was following the ASSERT_RTNL model, but that is used in quite deep
> and complex call stacks, and it is useful to scatter the macro in lots
> of places. PHY drivers are however very shallow, so yes, putting them
> in mdio_bus.c makes a lot of sense.
>
> > I don't see any point in using __FILE__ and __LINE__ in the above
> > macro either. Firstly, WARN_ONCE() already includes the file and line,
> > and secondly, the backtrace is more useful than the file and line where
> > the assertion occurs especially if it's placed in mdio_bus.c
>
> And PHY driver functions are simpler, there is a lot less inlining
> going on, so the function name is probably all you need to know to
> find where you messed up the locking. So i agree, they can be removed.
>
> Andrew
Hi Andrew and Russell, Thanks!
change the mdio_bus.c like follow ok?
-add "ASSERT_MDIO"
-add "ASSERT_MDIO(bus);" in __mdiobus_read and __mdiobus_write function
static inline bool mdiobus_is_locked(struct mii_bus *bus)
{
return mutex_is_locked(&bus->mdio_lock);
}
#define ASSERT_MDIO(bus) \
WARN_ONCE(!mdiobus_is_locked(bus), \
"MDIO: assertion failed\n")
int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
{
int retval;
ASSERT_MDIO(bus);
lockdep_assert_held_once(&bus->mdio_lock);
retval = bus->read(bus, addr, regnum);
...
}
int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
{
int err;
ASSERT_MDIO(bus);
lockdep_assert_held_once(&bus->mdio_lock);
err = bus->write(bus, addr, regnum, val);
...
}
on Dec. 2, 2022, 1:27 p.m. UTC , Andrew Lunn wrote:
> > /**
> > * ytphy_read_ext() - read a PHY's extended register
> > * @phydev: a pointer to a &struct phy_device
> > @@ -258,6 +271,8 @@ static int ytphy_read_ext(struct phy_device *phydev, u16 regnum)
> > {
> > int ret;
> >
> > + ASSERT_MDIO(phydev);
> > +
> > ret = __phy_write(phydev, YTPHY_PAGE_SELECT, regnum);
> > if (ret < 0)
> > return ret;
> > @@ -297,6 +312,8 @@ static int ytphy_write_ext(struct phy_device *phydev, u16 regnum, u16 val)
> > {
> > int ret;
> >
> > + ASSERT_MDIO(phydev);
> > +
> > ret = __phy_write(phydev, YTPHY_PAGE_SELECT, regnum);
> > if (ret < 0)
> > return ret;
> > @@ -342,6 +359,8 @@ static int ytphy_modify_ext(struct phy_device *phydev, u16 regnum, u16 mask,
> > {
> > int ret;
> >
> > + ASSERT_MDIO(phydev);
> > +
> > ret = __phy_write(phydev, YTPHY_PAGE_SELECT, regnum);
> > if (ret < 0)
> > return ret;
> > @@ -479,6 +498,76 @@ static int ytphy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
> > return phy_restore_page(phydev, old_page, ret);
> > }
>
> Please make the above one patch, which adds the macro and its
> users. There are a couple more below as well.
>
> Did it find any problems in the current code? Any fixes mixed
> in here?
>
> Then add yt8531 is another patch.
>
Thanks!
It not find any problems in the current code.
ASSERT_MDIO in motorcomm.c will be removed.
> > +/**
> > + * yt8531_set_wol() - turn wake-on-lan on or off
> > + * @phydev: a pointer to a &struct phy_device
> > + * @wol: a pointer to a &struct ethtool_wolinfo
> > + *
> > + * returns 0 or negative errno code
> > + */
> > +static int yt8531_set_wol(struct phy_device *phydev,
> > + struct ethtool_wolinfo *wol)
> > +{
> > + struct net_device *p_attached_dev;
> > + const u16 mac_addr_reg[] = {
> > + YTPHY_WOL_MACADDR2_REG,
> > + YTPHY_WOL_MACADDR1_REG,
> > + YTPHY_WOL_MACADDR0_REG,
> > + };
> > + const u8 *mac_addr;
> > + u16 mask;
> > + u16 val;
> > + int ret;
> > + u8 i;
> > +
> > + if (wol->wolopts & WAKE_MAGIC) {
> > + p_attached_dev = phydev->attached_dev;
> > + if (!p_attached_dev)
> > + return -ENODEV;
> > +
> > + mac_addr = (const u8 *)p_attached_dev->dev_addr;
>
> Why the cast?
I'm sorry. What does "Why the cast?" mean?
>
> > + if (!is_valid_ether_addr(mac_addr))
> > + return -EINVAL;
>
> Andrew
>
^ permalink raw reply
* [PATCH V2 0/2] fix mac not working after system resumed with WoL enabled
From: Clark Wang @ 2022-12-21 8:01 UTC (permalink / raw)
To: linux, peppe.cavallaro, alexandre.torgue, joabreu, davem,
edumazet, kuba, pabeni, mcoquelin.stm32, andrew, hkallweit1
Cc: netdev, linux-stm32, linux-arm-kernel, linux-kernel
Hi,
The issue description is in the commit message.
This patchset is the second version following discussions with Russell.
But the name of each patch has changed, so V2 is not marked in each patch.
Thanks.
Clark Wang (2):
net: phylink: add a function to resume phy alone to fix resume issue
with WoL enabled
net: stmmac: resume phy separately before calling stmmac_hw_setup()
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 16 +++++++-------
drivers/net/phy/phylink.c | 21 ++++++++++++++++++-
include/linux/phylink.h | 1 +
3 files changed, 28 insertions(+), 10 deletions(-)
--
2.34.1
^ permalink raw reply
* [PATCH 2/2] net: stmmac: resume phy separately before calling stmmac_hw_setup()
From: Clark Wang @ 2022-12-21 8:01 UTC (permalink / raw)
To: linux, peppe.cavallaro, alexandre.torgue, joabreu, davem,
edumazet, kuba, pabeni, mcoquelin.stm32, andrew, hkallweit1
Cc: netdev, linux-stm32, linux-arm-kernel, linux-kernel
In-Reply-To: <20221221080144.2549125-1-xiaoning.wang@nxp.com>
On some platforms, mac cannot work after resumed from the suspend with WoL
enabled.
We found the stmmac_hw_setup() when system resumes will called after the
stmmac_mac_link_up(). So the correct values set in stmmac_mac_link_up()
are overwritten by stmmac_core_init() in phylink_resume().
So call the new added function in phylink to resume phy fristly.
Then can call the stmmac_hw_setup() before calling phy_resume().
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
---
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index c6951c976f5d..d0bdc9b6dbe8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7532,16 +7532,9 @@ int stmmac_resume(struct device *dev)
}
rtnl_lock();
- if (device_may_wakeup(priv->device) && priv->plat->pmt) {
- phylink_resume(priv->phylink);
- } else {
- phylink_resume(priv->phylink);
- if (device_may_wakeup(priv->device))
- phylink_speed_up(priv->phylink);
- }
- rtnl_unlock();
- rtnl_lock();
+ phylink_phy_resume(priv->phylink);
+
mutex_lock(&priv->lock);
stmmac_reset_queues_param(priv);
@@ -7559,6 +7552,11 @@ int stmmac_resume(struct device *dev)
stmmac_enable_all_dma_irq(priv);
mutex_unlock(&priv->lock);
+
+ phylink_resume(priv->phylink);
+ if (device_may_wakeup(priv->device) && !priv->plat->pmt)
+ phylink_speed_up(priv->phylink);
+
rtnl_unlock();
netif_device_attach(ndev);
--
2.34.1
^ permalink raw reply related
* [PATCH 1/2] net: phylink: add a function to resume phy alone to fix resume issue with WoL enabled
From: Clark Wang @ 2022-12-21 8:01 UTC (permalink / raw)
To: linux, peppe.cavallaro, alexandre.torgue, joabreu, davem,
edumazet, kuba, pabeni, mcoquelin.stm32, andrew, hkallweit1
Cc: netdev, linux-stm32, linux-arm-kernel, linux-kernel
In-Reply-To: <20221221080144.2549125-1-xiaoning.wang@nxp.com>
Issue we met:
On some platforms, mac cannot work after resumed from the suspend with WoL
enabled.
The cause of the issue:
1. phylink_resolve() is in a workqueue which will not be executed immediately.
This is the call sequence:
phylink_resolve()->phylink_link_up()->pl->mac_ops->mac_link_up()
For stmmac driver, mac_link_up() will set the correct speed/duplex...
values which are from link_state.
2. In stmmac_resume(), it will call stmmac_hw_setup() after called the
phylink_resume(), because mac need phy rx_clk to do the reset.
stmmac_core_init() is called in function stmmac_hw_setup(), which will
reset the mac and set the speed/duplex... to default value.
Conclusion: Because phylink_resolve() cannot determine when it is called, it
cannot be guaranteed to be called after stmmac_core_init().
Once stmmac_core_init() is called after phylink_resolve(),
the mac will be misconfigured and cannot be used.
In order to avoid this problem, add a function called phylink_phy_resume()
to resume phy separately. This eliminates the need to call phylink_resume()
before stmmac_hw_setup().
Add another judgement before called phy_start() in phylink_start(). This way
phy_start() will not be called multiple times when resumes. At the same time,
it may not affect other drivers that do not use phylink_phy_resume().
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
---
drivers/net/phy/phylink.c | 21 ++++++++++++++++++++-
include/linux/phylink.h | 1 +
2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 09cc65c0da93..5bab59142579 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1939,7 +1939,7 @@ void phylink_start(struct phylink *pl)
}
if (poll)
mod_timer(&pl->link_poll, jiffies + HZ);
- if (pl->phydev)
+ if (pl->phydev && pl->phydev->state < PHY_UP)
phy_start(pl->phydev);
if (pl->sfp_bus)
sfp_upstream_start(pl->sfp_bus);
@@ -2020,6 +2020,25 @@ void phylink_suspend(struct phylink *pl, bool mac_wol)
}
EXPORT_SYMBOL_GPL(phylink_suspend);
+/**
+ * phylink_phy_resume() - resume phy alone
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ *
+ * In the MAC driver using phylink, if the MAC needs the clock of the phy
+ * when it resumes, can call this function to resume the phy separately.
+ * Then proceed to MAC resume operations.
+ */
+void phylink_phy_resume(struct phylink *pl)
+{
+ ASSERT_RTNL();
+
+ if (!test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)
+ && pl->phydev)
+ phy_start(pl->phydev);
+
+}
+EXPORT_SYMBOL_GPL(phylink_phy_resume);
+
/**
* phylink_resume() - handle a network device resume event
* @pl: a pointer to a &struct phylink returned from phylink_create()
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index c492c26202b5..6edfab5f754c 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -589,6 +589,7 @@ void phylink_stop(struct phylink *);
void phylink_suspend(struct phylink *pl, bool mac_wol);
void phylink_resume(struct phylink *pl);
+void phylink_phy_resume(struct phylink *pl);
void phylink_ethtool_get_wol(struct phylink *, struct ethtool_wolinfo *);
int phylink_ethtool_set_wol(struct phylink *, struct ethtool_wolinfo *);
--
2.34.1
^ permalink raw reply related
* [PATCH v3] usbnet: optimize usbnet_bh() to reduce CPU load
From: Leesoo Ahn @ 2022-12-21 7:59 UTC (permalink / raw)
To: lsahn
Cc: Oliver Neukum, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Greg KH, netdev, linux-usb, linux-kernel
The current source pushes skb into dev->done queue by calling
skb_queue_tail() and then pop it by calling skb_dequeue() to branch to
rx_cleanup state for freeing urb/skb in usbnet_bh(). It takes extra CPU
load, 2.21% (skb_queue_tail) as follows.
- 11.58% 0.26% swapper [k] usbnet_bh
- 11.32% usbnet_bh
- 6.43% skb_dequeue
6.34% _raw_spin_unlock_irqrestore
- 2.21% skb_queue_tail
2.19% _raw_spin_unlock_irqrestore
- 1.68% consume_skb
- 0.97% kfree_skbmem
0.80% kmem_cache_free
0.53% skb_release_data
To reduce the extra CPU load use return values jumping to rx_cleanup
state directly to free them instead of calling skb_queue_tail() and
skb_dequeue() for push/pop respectively.
- 7.87% 0.25% swapper [k] usbnet_bh
- 7.62% usbnet_bh
- 4.81% skb_dequeue
4.74% _raw_spin_unlock_irqrestore
- 1.75% consume_skb
- 0.98% kfree_skbmem
0.78% kmem_cache_free
0.58% skb_release_data
0.53% smsc95xx_rx_fixup
Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
---
v3:
- Replace return values with proper -ERR values in rx_process()
v2:
- Replace goto label with return statement to reduce goto entropy
- Add CPU load information by perf in commit message
v1 at:
https://patchwork.kernel.org/project/netdevbpf/patch/20221217161851.829497-1-lsahn@ooseel.net/
---
drivers/net/usb/usbnet.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 64a9a80b2309..98d594210df4 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -555,32 +555,30 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags)
/*-------------------------------------------------------------------------*/
-static inline void rx_process (struct usbnet *dev, struct sk_buff *skb)
+static inline int rx_process(struct usbnet *dev, struct sk_buff *skb)
{
if (dev->driver_info->rx_fixup &&
!dev->driver_info->rx_fixup (dev, skb)) {
/* With RX_ASSEMBLE, rx_fixup() must update counters */
if (!(dev->driver_info->flags & FLAG_RX_ASSEMBLE))
dev->net->stats.rx_errors++;
- goto done;
+ return -EPROTO;
}
// else network stack removes extra byte if we forced a short packet
/* all data was already cloned from skb inside the driver */
if (dev->driver_info->flags & FLAG_MULTI_PACKET)
- goto done;
+ return -EALREADY;
if (skb->len < ETH_HLEN) {
dev->net->stats.rx_errors++;
dev->net->stats.rx_length_errors++;
netif_dbg(dev, rx_err, dev->net, "rx length %d\n", skb->len);
- } else {
- usbnet_skb_return(dev, skb);
- return;
+ return -EPROTO;
}
-done:
- skb_queue_tail(&dev->done, skb);
+ usbnet_skb_return(dev, skb);
+ return 0;
}
/*-------------------------------------------------------------------------*/
@@ -1528,13 +1526,14 @@ static void usbnet_bh (struct timer_list *t)
entry = (struct skb_data *) skb->cb;
switch (entry->state) {
case rx_done:
- entry->state = rx_cleanup;
- rx_process (dev, skb);
+ if (rx_process(dev, skb))
+ goto cleanup;
continue;
case tx_done:
kfree(entry->urb->sg);
fallthrough;
case rx_cleanup:
+cleanup:
usb_free_urb (entry->urb);
dev_kfree_skb (skb);
continue;
--
2.34.1
^ permalink raw reply related
* Re: [PATCH net 1/1] ice: xsk: do not use xdp_return_frame() on tx_buf->raw_buf
From: Piotr Raczynski @ 2022-12-21 8:09 UTC (permalink / raw)
To: Tony Nguyen
Cc: davem, kuba, pabeni, edumazet, Maciej Fijalkowski, netdev,
magnus.karlsson, ast, daniel, hawk, john.fastabend, bpf,
Robin Cowley, Chandan Kumar Rout
In-Reply-To: <20221220175448.693999-1-anthony.l.nguyen@intel.com>
On Tue, Dec 20, 2022 at 09:54:48AM -0800, Tony Nguyen wrote:
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
>
> Previously ice XDP xmit routine was changed in a way that it avoids
> xdp_buff->xdp_frame conversion as it is simply not needed for handling
> XDP_TX action and what is more it saves us CPU cycles. This routine is
> re-used on ZC driver to handle XDP_TX action.
>
> Although for XDP_TX on Rx ZC xdp_buff that comes from xsk_buff_pool is
> converted to xdp_frame, xdp_frame itself is not stored inside
> ice_tx_buf, we only store raw data pointer. Casting this pointer to
> xdp_frame and calling against it xdp_return_frame in
> ice_clean_xdp_tx_buf() results in undefined behavior.
>
> To fix this, simply call page_frag_free() on tx_buf->raw_buf.
> Later intention is to remove the buff->frame conversion in order to
> simplify the codebase and improve XDP_TX performance on ZC.
>
> Fixes: 126cdfe1007a ("ice: xsk: Improve AF_XDP ZC Tx and use batching API")
> Reported-and-tested-by: Robin Cowley <robin.cowley@thehutgroup.com>
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> (A Contingent Worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
> drivers/net/ethernet/intel/ice/ice_xsk.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> index 907055b77af0..7105de6fb344 100644
> --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> @@ -783,7 +783,7 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> static void
> ice_clean_xdp_tx_buf(struct ice_tx_ring *xdp_ring, struct ice_tx_buf *tx_buf)
> {
> - xdp_return_frame((struct xdp_frame *)tx_buf->raw_buf);
> + page_frag_free(tx_buf->raw_buf);
> xdp_ring->xdp_tx_active--;
> dma_unmap_single(xdp_ring->dev, dma_unmap_addr(tx_buf, dma),
> dma_unmap_len(tx_buf, len), DMA_TO_DEVICE);
> --
> 2.35.1
>
LGTM
Reviewed-by: Piotr Raczynski <piotr.raczynski@.intel.com>
^ permalink raw reply
* [syzbot] BUG: corrupted list in nfc_llcp_register_device
From: syzbot @ 2022-12-21 8:14 UTC (permalink / raw)
To: davem, dvyukov, edumazet, krzysztof.kozlowski, kuba, linma,
linux-kernel, netdev, pabeni, syzkaller-bugs
Hello,
syzbot found the following issue on:
HEAD commit: 6feb57c2fd7c Merge tag 'kbuild-v6.2' of git://git.kernel.o..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=14dd1bbf880000
kernel config: https://syzkaller.appspot.com/x/.config?x=d3fb546de56fbf8d
dashboard link: https://syzkaller.appspot.com/bug?extid=c1d0a03d305972dbbe14
compiler: Debian clang version 13.0.1-++20220126092033+75e33f71c2da-1~exp1~20220126212112.63, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15fbcbd0480000
Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/81556e491789/disk-6feb57c2.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/065c943ec9de/vmlinux-6feb57c2.xz
kernel image: https://storage.googleapis.com/syzbot-assets/66e98c522c1f/bzImage-6feb57c2.xz
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+c1d0a03d305972dbbe14@syzkaller.appspotmail.com
list_add corruption. next->prev should be prev (ffffffff8e7c1b40), but was 054e024500005c15. (next=ffff8880286ef000).
------------[ cut here ]------------
kernel BUG at lib/list_debug.c:29!
invalid opcode: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 23580 Comm: syz-executor.3 Not tainted 6.1.0-syzkaller-13822-g6feb57c2fd7c #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022
RIP: 0010:__list_add_valid+0xdd/0x100 lib/list_debug.c:27
Code: b9 45 6b 06 0f 0b 48 c7 c7 20 c2 4b 8b 31 c0 e8 a9 45 6b 06 0f 0b 48 c7 c7 80 c2 4b 8b 4c 89 e6 4c 89 f1 31 c0 e8 93 45 6b 06 <0f> 0b 48 c7 c7 00 c3 4b 8b 4c 89 f6 4c 89 e1 31 c0 e8 7d 45 6b 06
RSP: 0018:ffffc9000bb8f560 EFLAGS: 00010246
RAX: 0000000000000075 RBX: ffff8880286ef008 RCX: 7255f226623a9300
RDX: 0000000000000000 RSI: 0000000080000000 RDI: 0000000000000000
RBP: 1ffff110056b8600 R08: ffffffff816f2c9d R09: fffff52001771e65
R10: fffff52001771e65 R11: 1ffff92001771e64 R12: ffffffff8e7c1b40
R13: dffffc0000000000 R14: ffff8880286ef000 R15: ffff88802b5c3000
FS: 00007fa56cb80700(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f9560bfe718 CR3: 0000000078b31000 CR4: 00000000003506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
__list_add include/linux/list.h:69 [inline]
list_add include/linux/list.h:88 [inline]
nfc_llcp_register_device+0x6c4/0x800 net/nfc/llcp_core.c:1603
nfc_register_device+0x68/0x320 net/nfc/core.c:1124
nci_register_device+0x7b5/0x8f0 net/nfc/nci/core.c:1257
virtual_ncidev_open+0x138/0x1b0 drivers/nfc/virtual_ncidev.c:148
misc_open+0x346/0x3c0 drivers/char/misc.c:165
chrdev_open+0x53b/0x5f0 fs/char_dev.c:414
do_dentry_open+0x85f/0x11b0 fs/open.c:882
do_open fs/namei.c:3557 [inline]
path_openat+0x25ba/0x2dd0 fs/namei.c:3714
do_filp_open+0x264/0x4f0 fs/namei.c:3741
do_sys_openat2+0x124/0x4e0 fs/open.c:1310
do_sys_open fs/open.c:1326 [inline]
__do_sys_openat fs/open.c:1342 [inline]
__se_sys_openat fs/open.c:1337 [inline]
__x64_sys_openat+0x243/0x290 fs/open.c:1337
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7fa56be8c0d9
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 f1 19 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fa56cb80168 EFLAGS: 00000246 ORIG_RAX: 0000000000000101
RAX: ffffffffffffffda RBX: 00007fa56bfabf80 RCX: 00007fa56be8c0d9
RDX: 0000000000000002 RSI: 0000000020000080 RDI: ffffffffffffff9c
RBP: 00007fa56bee7ae9 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 00007ffd25093fef R14: 00007fa56cb80300 R15: 0000000000022000
</TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:__list_add_valid+0xdd/0x100 lib/list_debug.c:27
Code: b9 45 6b 06 0f 0b 48 c7 c7 20 c2 4b 8b 31 c0 e8 a9 45 6b 06 0f 0b 48 c7 c7 80 c2 4b 8b 4c 89 e6 4c 89 f1 31 c0 e8 93 45 6b 06 <0f> 0b 48 c7 c7 00 c3 4b 8b 4c 89 f6 4c 89 e1 31 c0 e8 7d 45 6b 06
RSP: 0018:ffffc9000bb8f560 EFLAGS: 00010246
RAX: 0000000000000075 RBX: ffff8880286ef008 RCX: 7255f226623a9300
RDX: 0000000000000000 RSI: 0000000080000000 RDI: 0000000000000000
RBP: 1ffff110056b8600 R08: ffffffff816f2c9d R09: fffff52001771e65
R10: fffff52001771e65 R11: 1ffff92001771e64 R12: ffffffff8e7c1b40
R13: dffffc0000000000 R14: ffff8880286ef000 R15: ffff88802b5c3000
FS: 00007fa56cb80700(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f9560bfe718 CR3: 0000000078b31000 CR4: 00000000003506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* Livraison d'emballage
From: Timeo Moreau @ 2022-12-21 9:10 UTC (permalink / raw)
To: netdev
Bonjour,
Je vous contacte au nom d'une entreprise qui a de nombreuses années d'expérience dans l'industrie de l'emballage.
Nous produisons des matériaux d'emballage en feuille modernes, imprimés en technologie flexographique.
Nous exécutons les commandes en coopération avec les fournisseurs de matières premières les plus importants et réputés, garantissant une qualité stable et reproductible des produits proposés.
Les produits dans nos emballages sont disponibles dans les plus grandes chaînes de magasins, ce qui est la meilleure preuve de la haute qualité de nos emballages.
Souhaitez-vous parler d'opportunités de coopération?
Timeo Moreau
^ permalink raw reply
* [PATCH net] net: lan966x: Fix configuration of the PCS
From: Horatiu Vultur @ 2022-12-21 9:33 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: davem, edumazet, kuba, pabeni, UNGLinuxDriver, linux,
Horatiu Vultur
When the PCS was taken out of reset, we were changing by mistake also
the speed to 100 Mbit. But in case the link was going down, the link
up routine was setting correctly the link speed. If the link was not
getting down then the speed was forced to run at 100 even if the
speed was something else.
On lan966x, to set the speed link to 1G or 2.5G a value of 1 needs to be
written in DEV_CLOCK_CFG_LINK_SPEED. This is similar to the procedure in
lan966x_port_init.
The issue was reproduced using 1000base-x sfp module using the commands:
ip link set dev eth2 up
ip link addr add 10.97.10.2/24 dev eth2
ethtool -s eth2 speed 1000 autoneg off
Fixes: d28d6d2e37d1 ("net: lan966x: add port module support")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
drivers/net/ethernet/microchip/lan966x/lan966x_port.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_port.c b/drivers/net/ethernet/microchip/lan966x/lan966x_port.c
index 1a61c6cdb0779..0050fcb988b75 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_port.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_port.c
@@ -381,7 +381,7 @@ int lan966x_port_pcs_set(struct lan966x_port *port,
}
/* Take PCS out of reset */
- lan_rmw(DEV_CLOCK_CFG_LINK_SPEED_SET(2) |
+ lan_rmw(DEV_CLOCK_CFG_LINK_SPEED_SET(LAN966X_SPEED_1000) |
DEV_CLOCK_CFG_PCS_RX_RST_SET(0) |
DEV_CLOCK_CFG_PCS_TX_RST_SET(0),
DEV_CLOCK_CFG_LINK_SPEED |
--
2.38.0
^ permalink raw reply related
* [PATCHv2 net-next] sched: multicast sched extack messages
From: Hangbin Liu @ 2022-12-21 9:39 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, David Ahern,
Hangbin Liu, Marcelo Ricardo Leitner
In commit 81c7288b170a ("sched: cls: enable verbose logging") Marcelo
made cls could log verbose info for offloading failures, which helps
improving Open vSwitch debuggability when using flower offloading.
It would also be helpful if "tc monitor" could log this message, as it
doesn't require vswitchd log level adjusment. Let's add a new function
to report the extack message so the monitor program could receive the
failures.
e.g.
# tc monitor
added chain dev enp3s0f1np1 parent ffff: chain 0
added filter dev enp3s0f1np1 ingress protocol all pref 49152 flower chain 0 handle 0x1
ct_state +trk+new
not_in_hw
action order 1: gact action drop
random type none pass val 0
index 1 ref 1 bind 1
Warning: mlx5_core: matching on ct_state +new isn't supported.
Suggested-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
v2: use NLMSG_ERROR instad of NLMSG_DONE to report the extack message
---
net/sched/cls_api.c | 61 ++++++++++++++++++++++++++++++++++-----------
1 file changed, 47 insertions(+), 14 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 668130f08903..a63262f0dc2c 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1813,11 +1813,39 @@ static struct tcf_proto *tcf_chain_tp_find(struct tcf_chain *chain,
return tp;
}
+static int tfilter_set_nl_ext(struct sk_buff *skb, const struct nlmsghdr *n,
+ struct netlink_ext_ack *extack, u32 portid)
+{
+ struct nlmsgerr *errmsg;
+ struct nlmsghdr *nlh;
+
+ if (!extack || !extack->_msg)
+ return 0;
+
+ nlh = nlmsg_put(skb, portid, n->nlmsg_seq, NLMSG_ERROR, sizeof(*errmsg),
+ NLM_F_ACK_TLVS | NLM_F_CAPPED);
+ if (!nlh)
+ return -1;
+
+ errmsg = (struct nlmsgerr *)nlmsg_data(nlh);
+ errmsg->error = 0;
+ errmsg->msg = *n;
+
+ if (nla_put_string(skb, NLMSGERR_ATTR_MSG, extack->_msg))
+ return -1;
+
+ nlmsg_end(skb, nlh);
+
+ return 0;
+}
+
static int tcf_fill_node(struct net *net, struct sk_buff *skb,
struct tcf_proto *tp, struct tcf_block *block,
struct Qdisc *q, u32 parent, void *fh,
u32 portid, u32 seq, u16 flags, int event,
- bool terse_dump, bool rtnl_held)
+ bool terse_dump, bool rtnl_held,
+ const struct nlmsghdr *n,
+ struct netlink_ext_ack *extack)
{
struct tcmsg *tcm;
struct nlmsghdr *nlh;
@@ -1858,6 +1886,10 @@ static int tcf_fill_node(struct net *net, struct sk_buff *skb,
goto nla_put_failure;
}
nlh->nlmsg_len = skb_tail_pointer(skb) - b;
+
+ if ((flags & NLM_F_ACK) && tfilter_set_nl_ext(skb, n, extack, portid))
+ goto out_nlmsg_trim;
+
return skb->len;
out_nlmsg_trim:
@@ -1871,7 +1903,7 @@ static int tfilter_notify(struct net *net, struct sk_buff *oskb,
struct nlmsghdr *n, struct tcf_proto *tp,
struct tcf_block *block, struct Qdisc *q,
u32 parent, void *fh, int event, bool unicast,
- bool rtnl_held)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
struct sk_buff *skb;
u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
@@ -1883,7 +1915,7 @@ static int tfilter_notify(struct net *net, struct sk_buff *oskb,
if (tcf_fill_node(net, skb, tp, block, q, parent, fh, portid,
n->nlmsg_seq, n->nlmsg_flags, event,
- false, rtnl_held) <= 0) {
+ false, rtnl_held, n, extack) <= 0) {
kfree_skb(skb);
return -EINVAL;
}
@@ -1912,7 +1944,7 @@ static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
if (tcf_fill_node(net, skb, tp, block, q, parent, fh, portid,
n->nlmsg_seq, n->nlmsg_flags, RTM_DELTFILTER,
- false, rtnl_held) <= 0) {
+ false, rtnl_held, n, extack) <= 0) {
NL_SET_ERR_MSG(extack, "Failed to build del event notification");
kfree_skb(skb);
return -EINVAL;
@@ -1938,14 +1970,15 @@ static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb,
struct tcf_block *block, struct Qdisc *q,
u32 parent, struct nlmsghdr *n,
- struct tcf_chain *chain, int event)
+ struct tcf_chain *chain, int event,
+ struct netlink_ext_ack *extack)
{
struct tcf_proto *tp;
for (tp = tcf_get_next_proto(chain, NULL);
tp; tp = tcf_get_next_proto(chain, tp))
- tfilter_notify(net, oskb, n, tp, block,
- q, parent, NULL, event, false, true);
+ tfilter_notify(net, oskb, n, tp, block, q, parent, NULL,
+ event, false, true, extack);
}
static void tfilter_put(struct tcf_proto *tp, void *fh)
@@ -2156,7 +2189,7 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
flags, extack);
if (err == 0) {
tfilter_notify(net, skb, n, tp, block, q, parent, fh,
- RTM_NEWTFILTER, false, rtnl_held);
+ RTM_NEWTFILTER, false, rtnl_held, extack);
tfilter_put(tp, fh);
/* q pointer is NULL for shared blocks */
if (q)
@@ -2284,7 +2317,7 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
if (prio == 0) {
tfilter_notify_chain(net, skb, block, q, parent, n,
- chain, RTM_DELTFILTER);
+ chain, RTM_DELTFILTER, extack);
tcf_chain_flush(chain, rtnl_held);
err = 0;
goto errout;
@@ -2308,7 +2341,7 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
tcf_proto_put(tp, rtnl_held, NULL);
tfilter_notify(net, skb, n, tp, block, q, parent, fh,
- RTM_DELTFILTER, false, rtnl_held);
+ RTM_DELTFILTER, false, rtnl_held, extack);
err = 0;
goto errout;
}
@@ -2452,7 +2485,7 @@ static int tc_get_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
err = -ENOENT;
} else {
err = tfilter_notify(net, skb, n, tp, block, q, parent,
- fh, RTM_NEWTFILTER, true, rtnl_held);
+ fh, RTM_NEWTFILTER, true, rtnl_held, extack);
if (err < 0)
NL_SET_ERR_MSG(extack, "Failed to send filter notify message");
}
@@ -2490,7 +2523,7 @@ static int tcf_node_dump(struct tcf_proto *tp, void *n, struct tcf_walker *arg)
return tcf_fill_node(net, a->skb, tp, a->block, a->q, a->parent,
n, NETLINK_CB(a->cb->skb).portid,
a->cb->nlh->nlmsg_seq, NLM_F_MULTI,
- RTM_NEWTFILTER, a->terse_dump, true);
+ RTM_NEWTFILTER, a->terse_dump, true, a->cb->nlh, NULL);
}
static bool tcf_chain_dump(struct tcf_chain *chain, struct Qdisc *q, u32 parent,
@@ -2524,7 +2557,7 @@ static bool tcf_chain_dump(struct tcf_chain *chain, struct Qdisc *q, u32 parent,
if (tcf_fill_node(net, skb, tp, block, q, parent, NULL,
NETLINK_CB(cb->skb).portid,
cb->nlh->nlmsg_seq, NLM_F_MULTI,
- RTM_NEWTFILTER, false, true) <= 0)
+ RTM_NEWTFILTER, false, true, cb->nlh, NULL) <= 0)
goto errout;
cb->args[1] = 1;
}
@@ -2912,7 +2945,7 @@ static int tc_ctl_chain(struct sk_buff *skb, struct nlmsghdr *n,
break;
case RTM_DELCHAIN:
tfilter_notify_chain(net, skb, block, q, parent, n,
- chain, RTM_DELTFILTER);
+ chain, RTM_DELTFILTER, extack);
/* Flush the chain first as the user requested chain removal. */
tcf_chain_flush(chain, true);
/* In case the chain was successfully deleted, put a reference
--
2.38.1
^ permalink raw reply related
* [PATCH net v2] net: phy: mxl-gpy: fix delay time required by loopback disable function
From: Xu Liang @ 2022-12-21 9:43 UTC (permalink / raw)
To: andrew, hkallweit1, netdev, davem, kuba
Cc: linux, hmehrtens, tmohren, mohammad.athari.ismail, edumazet,
pabeni, Xu Liang
GPY2xx devices need 3 seconds to fully switch out of loopback mode
before it can safely re-enter loopback mode.
Signed-off-by: Xu Liang <lxu@maxlinear.com>
---
drivers/net/phy/mxl-gpy.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index 147d7a5a9b35..b682d7fc477c 100644
--- a/drivers/net/phy/mxl-gpy.c
+++ b/drivers/net/phy/mxl-gpy.c
@@ -770,9 +770,11 @@ static int gpy_loopback(struct phy_device *phydev, bool enable)
enable ? BMCR_LOOPBACK : 0);
if (!ret) {
/* It takes some time for PHY device to switch
- * into/out-of loopback mode.
+ * into/out-of loopback mode. It takes 3 seconds
+ * to fully switch out of loopback mode before
+ * it can safely re-enter loopback mode.
*/
- msleep(100);
+ msleep(enable ? 100 : 3000);
}
return ret;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] nfc: Fix potential resource leaks
From: Krzysztof Kozlowski @ 2022-12-21 9:45 UTC (permalink / raw)
To: Miaoqian Lin, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Samuel Ortiz, Christophe Ricard, netdev,
linux-kernel
In-Reply-To: <20221220134623.2084443-1-linmq006@gmail.com>
On 20/12/2022 14:46, Miaoqian Lin wrote:
> nfc_get_device() take reference for the device, add missing
> nfc_put_device() to release it when not need anymore.
> Also fix the style warnning by use error EOPNOTSUPP instead of
> ENOTSUPP.
>
> Fixes: 5ce3f32b5264 ("NFC: netlink: SE API implementation")
> Fixes: 29e76924cf08 ("nfc: netlink: Add capability to reply to vendor_cmd with data")
> Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
> ---
> net/nfc/netlink.c | 51 ++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/net/nfc/netlink.c b/net/nfc/netlink.c
> index 9d91087b9399..d081beaf4828 100644
> --- a/net/nfc/netlink.c
> +++ b/net/nfc/netlink.c
> @@ -1497,6 +1497,7 @@ static int nfc_genl_se_io(struct sk_buff *skb, struct genl_info *info)
> u32 dev_idx, se_idx;
> u8 *apdu;
> size_t apdu_len;
> + int error;
Let's don't introduce the third or fourth style. Existing code calls it
"rc".
>
> if (!info->attrs[NFC_ATTR_DEVICE_INDEX] ||
> !info->attrs[NFC_ATTR_SE_INDEX] ||
> @@ -1510,25 +1511,37 @@ static int nfc_genl_se_io(struct sk_buff *skb, struct genl_info *info)
> if (!dev)
> return -ENODEV;
>
> - if (!dev->ops || !dev->ops->se_io)
> - return -ENOTSUPP;
> + if (!dev->ops || !dev->ops->se_io) {
> + error = -EOPNOTSUPP;
> + goto put_dev;
> + }
>
> apdu_len = nla_len(info->attrs[NFC_ATTR_SE_APDU]);
> - if (apdu_len == 0)
> - return -EINVAL;
> + if (apdu_len == 0) {
> + error = -EINVAL;
> + goto put_dev;
> + }
>
> apdu = nla_data(info->attrs[NFC_ATTR_SE_APDU]);
> - if (!apdu)
> - return -EINVAL;
> + if (!apdu) {
> + error = -EINVAL;
> + goto put_dev;
> + }
>
> ctx = kzalloc(sizeof(struct se_io_ctx), GFP_KERNEL);
> - if (!ctx)
> - return -ENOMEM;
> + if (!ctx) {
> + error = -ENOMEM;
> + goto put_dev;
> + }
>
> ctx->dev_idx = dev_idx;
> ctx->se_idx = se_idx;
>
> - return nfc_se_io(dev, se_idx, apdu, apdu_len, se_io_cb, ctx);
> + error = nfc_se_io(dev, se_idx, apdu, apdu_len, se_io_cb, ctx);
> +
> +put_dev:
> + nfc_put_device(dev);
> + return error;
> }
>
> static int nfc_genl_vendor_cmd(struct sk_buff *skb,
> @@ -1551,14 +1564,20 @@ static int nfc_genl_vendor_cmd(struct sk_buff *skb,
> subcmd = nla_get_u32(info->attrs[NFC_ATTR_VENDOR_SUBCMD]);
>
> dev = nfc_get_device(dev_idx);
> - if (!dev || !dev->vendor_cmds || !dev->n_vendor_cmds)
> + if (!dev)
> return -ENODEV;
Blank line
> + if (!dev->vendor_cmds || !dev->n_vendor_cmds) {
> + err = -ENODEV;
> + goto put_dev;
> + }
>
> if (info->attrs[NFC_ATTR_VENDOR_DATA]) {
> data = nla_data(info->attrs[NFC_ATTR_VENDOR_DATA]);
> data_len = nla_len(info->attrs[NFC_ATTR_VENDOR_DATA]);
> - if (data_len == 0)
> - return -EINVAL;
> + if (data_len == 0) {
> + err = -EINVAL;
> + goto put_dev;
> + }
> } else {
> data = NULL;
> data_len = 0;
> @@ -1573,10 +1592,14 @@ static int nfc_genl_vendor_cmd(struct sk_buff *skb,
> dev->cur_cmd_info = info;
> err = cmd->doit(dev, data, data_len);
> dev->cur_cmd_info = NULL;
> - return err;
> + goto put_dev;
> }
>
> - return -EOPNOTSUPP;
> + err = -EOPNOTSUPP;
> +
> +put_dev:
> + nfc_put_device(dev);
> + return err;
> }
>
> /* message building helper */
Best regards,
Krzysztof
^ permalink raw reply
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