Re: [PATCH net v2] ethernet: s2io: don't call dev_kfree_skb() under spin_lock_irqsave()

[Leon Romanovsky <leon@kernel.org>]

On Thu, Dec 08, 2022 at 05:03:20PM +0800, Yang Yingliang wrote:
> 
> On 2022/12/8 16:51, Leon Romanovsky wrote:
> > On Thu, Dec 08, 2022 at 04:40:35PM +0800, Yang Yingliang wrote:
> > > On 2022/12/8 15:56, Leon Romanovsky wrote:
> > > > On Wed, Dec 07, 2022 at 09:25:40AM +0800, Yang Yingliang wrote:
> > > > > It is not allowed to call consume_skb() from hardware interrupt context
> > > > > or with interrupts being disabled. So replace dev_kfree_skb() with
> > > > > dev_consume_skb_irq() under spin_lock_irqsave().
> > > > > 
> > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> > > > > ---
> > > > > v1 -> v2:
> > > > >     Add fix tag.
> > > > > ---
> > > > >    drivers/net/ethernet/neterion/s2io.c | 2 +-
> > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/net/ethernet/neterion/s2io.c b/drivers/net/ethernet/neterion/s2io.c
> > > > > index 1d3c4474b7cb..a83d61d45936 100644
> > > > > --- a/drivers/net/ethernet/neterion/s2io.c
> > > > > +++ b/drivers/net/ethernet/neterion/s2io.c
> > > > > @@ -2386,7 +2386,7 @@ static void free_tx_buffers(struct s2io_nic *nic)
> > > > >    			skb = s2io_txdl_getskb(&mac_control->fifos[i], txdp, j);
> > > > >    			if (skb) {
> > > > >    				swstats->mem_freed += skb->truesize;
> > > > > -				dev_kfree_skb(skb);
> > > > > +				dev_consume_skb_irq(skb);
> > > > And why did you use dev_consume_skb_irq() and not dev_kfree_skb_irq()?
> > > I chose dev_consume_skb_irq(), because dev_kfree_skb() is consume_skb().
> > Your commit message, title and actual change are totally misleading.
> > You replaced *_kfree_* with *_consume_* while talking about running it
> > in interrupts disabled context.
> I didn't mention dev_kfree_skb() is same as consume_skb(), I can add it to
> my
> commit message and send a new version.

I need you to send patch with commit message which aligns to the actual change.
Right now, it is not.

Thanks

> 
> Thanks,
> Yang
> > 
> > Thanks
> > 
> > > Thanks,
> > > Yang
> > > > Thanks
> > > > 
> > > > >    				cnt++;
> > > > >    			}
> > > > >    		}
> > > > > -- 
> > > > > 2.25.1
> > > > > 
> > > > .
> > .