| Message ID | 20240816204808.30359-1-davthompson@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v1] mlxbf_gige: disable port during stop() | expand |
On 2024-08-16 16:48 -0400, David Thompson wrote: > The mlxbf_gige_open() routine initializes and enables the > Gigabit Ethernet port from a hardware point of view. The > mlxbf_gige_stop() routine must disable the port hardware > to fully quiesce it. > > Fixes: f92e1869d74e ("Add Mellanox BlueField Gigabit Ethernet driver") > Reviewed-by: Asmaa Mnebhi <asmaa@nvidia.com> > Signed-off-by: David Thompson <davthompson@nvidia.com> > --- > drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c > index 385a56ac7348..12942a50e1bb 100644 > --- a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c > +++ b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c > @@ -205,8 +205,14 @@ static int mlxbf_gige_open(struct net_device *netdev) > static int mlxbf_gige_stop(struct net_device *netdev) > { > struct mlxbf_gige *priv = netdev_priv(netdev); > + u64 control; > + > + control = readq(priv->base + MLXBF_GIGE_CONTROL); > + control &= ~MLXBF_GIGE_CONTROL_PORT_EN; > + writeq(control, priv->base + MLXBF_GIGE_CONTROL); > > writeq(0, priv->base + MLXBF_GIGE_INT_EN); > + mb(); checkpatch says: WARNING: memory barrier without comment #37: FILE: drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c:215: + mb(); Is this memory barrier paired with another one?
On Mon, 19 Aug 2024 14:55:12 -0400 Benjamin Poirier wrote:
> Is this memory barrier paired with another one?
+1
You probably want synchronize_irq() here?
You should explain in the cover letter, the mb() seems to not be
mentioned.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Monday, August 19, 2024 8:47 PM > To: David Thompson <davthompson@nvidia.com> > Cc: Benjamin Poirier <benjamin.poirier@gmail.com>; davem@davemloft.net; > edumazet@google.com; pabeni@redhat.com; > andriy.shevchenko@linux.intel.com; u.kleine-koenig@pengutronix.de; Asmaa > Mnebhi <asmaa@nvidia.com>; netdev@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH net v1] mlxbf_gige: disable port during stop() > > On Mon, 19 Aug 2024 14:55:12 -0400 Benjamin Poirier wrote: > > Is this memory barrier paired with another one? > > +1 > You probably want synchronize_irq() here? > You should explain in the cover letter, the mb() seems to not be mentioned. > -- > pw-bot: cr Hello Jakub and Benjamin, thanks for your input. I will post a v2 adding information about the "mb()" call. Regarding the "synchronize_irq()" call, I did some research and noticed that some already merged driver patches (see list below for a sample) state that "synchronize_irq()" is unnecessary before "free_irq()": 3e0fcb782a9f i40e: Remove unnecessary synchronize_irq() before free_irq() 845517ed04ae RDMA/qedr: Remove unnecessary synchronize_irq() before free_irq() d1e7f009bfff net: qede: Remove unnecessary synchronize_irq() before free_irq() bd81bfb5a1d1 net: vxge: Remove unnecessary synchronize_irq() before free_irq() 29fd3ca1779f qed: Remove unnecessary synchronize_irq() before free_irq() 411dccf9d271 dmaengine: idxd: Remove unnecessary synchronize_irq() before free_irq() d887ae3247e0 octeontx2-pf: Remove unnecessary synchronize_irq() before free_irq() Given the above information, does my mlxbf_gige driver patch still need to invoke "synchronize_irq()" in the stop() method? The "mlxbf_gige_free_irq()" call within the stop() method invokes "free_irq()" for each of the driver's IRQs, so sounds like this "synchronize_irq()" is implicitly being invoked? Regards, Dave
On Wed, 28 Aug 2024 21:37:06 +0000 David Thompson wrote: > Hello Jakub and Benjamin, thanks for your input. > > I will post a v2 adding information about the "mb()" call. Perhaps this information you're adding will shed more light.. > Given the above information, does my mlxbf_gige driver patch still need to > invoke "synchronize_irq()" in the stop() method? The "mlxbf_gige_free_irq()" > call within the stop() method invokes "free_irq()" for each of the driver's IRQs, so > sounds like this "synchronize_irq()" is implicitly being invoked? I was talking about the point in which you add the mb(). IDK what you're trying to protect from but it's after what looks like disabling IRQ, and there's not free_irq() in that spot.
diff --git a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c index 385a56ac7348..12942a50e1bb 100644 --- a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c +++ b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c @@ -205,8 +205,14 @@ static int mlxbf_gige_open(struct net_device *netdev) static int mlxbf_gige_stop(struct net_device *netdev) { struct mlxbf_gige *priv = netdev_priv(netdev); + u64 control; + + control = readq(priv->base + MLXBF_GIGE_CONTROL); + control &= ~MLXBF_GIGE_CONTROL_PORT_EN; + writeq(control, priv->base + MLXBF_GIGE_CONTROL); writeq(0, priv->base + MLXBF_GIGE_INT_EN); + mb(); netif_stop_queue(netdev); napi_disable(&priv->napi); netif_napi_del(&priv->napi);
The mlxbf_gige_open() routine initializes and enables the Gigabit Ethernet port from a hardware point of view. The mlxbf_gige_stop() routine must disable the port hardware to fully quiesce it. Fixes: f92e1869d74e ("Add Mellanox BlueField Gigabit Ethernet driver") Reviewed-by: Asmaa Mnebhi <asmaa@nvidia.com> Signed-off-by: David Thompson <davthompson@nvidia.com> --- drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c | 6 ++++++ 1 file changed, 6 insertions(+)