netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: "Andrew Lunn" <andrew@lunn.ch>, "Íñigo Huguet" <ihuguet@redhat.com>
Cc: irusskikh@marvell.com, dbogdanov@marvell.com,
	davem@davemloft.net, edumazet@google.com, pabeni@redhat.com,
	netdev@vger.kernel.org, Li Liang <liali@redhat.com>
Subject: Re: [PATCH net] atlantic: fix deadlock at aq_nic_stop
Date: Mon, 17 Oct 2022 19:44:04 -0700	[thread overview]
Message-ID: <20221017194404.0f841a52@kernel.org> (raw)
In-Reply-To: <Y03y/D8WszbjmSwZ@lunn.ch>

On Tue, 18 Oct 2022 02:27:40 +0200 Andrew Lunn wrote:
> > > Please try to identify what is being protected. If it is driver
> > > internal state, could it be replaced with a driver mutex, rather than
> > > RTNL? Or is it network stack as a whole state, which really does
> > > require RTNL? If so, how do other drivers deal with this problem? Is
> > > it specific to MACSEC? Does MACSEC have a design problem?  
> > 
> > I already considered this possibility but discarded it because, as I
> > say above, everything else is already legitimately protected by
> > rtnl_lock.  
> 
> Did you look at other drivers using MACSEC offload? Is this driver
> unique in having stuff run in a work queue which you need to cancel?
> In fact, it is not limited to MACSEC, it could be any work queue which
> holds RTNL and needs to be cancelled.

FWIW the work APIs return a boolean to tell you if the work was
actually scheduled / canceled, and you can pair that with a reference
count of the netdev to avoid the typical _sync issues.

trigger()
	ASSERT_RTNL();
	if (schedule_work(netdev_priv->bla))
		netdev_hold();

work()
	rtnl_lock();
	if (netif_running())
		do_ya_thing();
	netdev_put();
	rtnl_unlock();

stop()
	ASSERT_RTNL();
	if (cancel_work(bla))
		netdev_put();

I think.

  reply	other threads:[~2022-10-18  2:44 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-14 10:34 [PATCH net] atlantic: fix deadlock at aq_nic_stop Íñigo Huguet
2022-10-14 12:13 ` Andrew Lunn
2022-10-14 12:43   ` Íñigo Huguet
2022-10-14 13:35     ` Andrew Lunn
2022-10-14 13:44       ` Íñigo Huguet
2022-10-15 15:09         ` Andrew Lunn
2022-10-17  7:22           ` Íñigo Huguet
2022-10-18  0:27             ` Andrew Lunn
2022-10-18  2:44               ` Jakub Kicinski [this message]
2022-10-18  6:15                 ` Íñigo Huguet
2022-10-18 15:59                   ` Jakub Kicinski
2022-10-19  6:18                     ` Íñigo Huguet
2022-10-19 15:39                       ` Jakub Kicinski
2022-10-20  7:46                         ` Íñigo Huguet
2022-10-18  6:11               ` Íñigo Huguet
2022-10-20  7:53 ` [PATCH v2 " Íñigo Huguet
2022-10-20  8:55   ` Igor Russkikh
2022-10-20 16:17     ` Íñigo Huguet
2022-10-21 19:01     ` Íñigo Huguet
2022-10-24  9:30   ` patchwork-bot+netdevbpf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20221017194404.0f841a52@kernel.org \
    --to=kuba@kernel.org \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=dbogdanov@marvell.com \
    --cc=edumazet@google.com \
    --cc=ihuguet@redhat.com \
    --cc=irusskikh@marvell.com \
    --cc=liali@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).