net: dsa: mv88e6xxx: don't dispose of Global2 IRQ mappings from mdiobus code

  From: Vladimir Oltean <vladimir.oltean@nxp.com>

irq_find_mapping() does not need irq_dispose_mapping(), only
irq_create_mapping() does.

Calling irq_dispose_mapping() from mv88e6xxx_g2_irq_mdio_free() and from
the error path of mv88e6xxx_g2_irq_mdio_setup() effectively means that
the mdiobus logic (for internal PHY interrupts) is disposing of a
hwirq->virq mapping which it is not responsible of (but instead, the
function pair mv88e6xxx_g2_irq_setup() + mv88e6xxx_g2_irq_free() is).

With the current code structure, this isn't such a huge problem, because
mv88e6xxx_g2_irq_mdio_free() is called relatively close to the real
owner of the IRQ mappings:

mv88e6xxx_remove()
-> mv88e6xxx_unregister_switch()
-> mv88e6xxx_mdios_unregister()
   -> mv88e6xxx_g2_irq_mdio_free()
-> mv88e6xxx_g2_irq_free()

and the switch isn't 'live' in any way such that it would be able of
generating interrupts at this point (mv88e6xxx_unregister_switch() has
been called).

However, there is a desire to split mv88e6xxx_mdios_unregister() and
mv88e6xxx_g2_irq_free() such that mv88e6xxx_mdios_unregister() only gets
called from mv88e6xxx_teardown(). This is much more problematic, as can
be seen below.

In a cross-chip scenario (say 3 switches d0032004.mdio-mii:10,
d0032004.mdio-mii:11 and d0032004.mdio-mii:12 which form a single DSA
tree), it is possible to unbind the device driver from a single switch
(say d0032004.mdio-mii:10).

When that happens, mv88e6xxx_remove() will be called for just that one
switch, and this will call mv88e6xxx_unregister_switch() which will tear
down the entire tree (calling mv88e6xxx_teardown() for all 3 switches).

Assuming mv88e6xxx_mdios_unregister() was moved to mv88e6xxx_teardown(),
at this stage, all 3 switches will have called irq_dispose_mapping() on
their mdiobus virqs.

When we bind again the device driver to d0032004.mdio-mii:10,
mv88e6xxx_probe() is called for it, which calls dsa_register_switch().
The DSA tree is now complete again, and mv88e6xxx_setup() is called for
all 3 switches.

Also assuming that mv88e6xxx_mdios_register() is moved to
mv88e6xxx_setup() (the 2 assumptions go together), at this point,
d0032004.mdio-mii:11 and d0032004.mdio-mii:12 don't have an IRQ mapping
for the internal PHYs anymore, as they've disposed of it in
mv88e6xxx_teardown(). Whereas switch d0032004.mdio-mii:10 has re-created
it, because its code path comes from mv88e6xxx_probe().

Simply put, this change prepares the driver to handle the movement of
mv88e6xxx_mdios_register() to mv88e6xxx_setup() for cross-chip DSA trees.

Also, the code being deleted was partially wrong anyway (in a way which
may have hidden this other issue). mv88e6xxx_g2_irq_mdio_setup()
populates bus->irq[] starting with offset chip->info->phy_base_addr, but
the teardown path doesn't apply that offset too. So it disposes of virq
0 for phy = [ 0, phy_base_addr ).

All switch families have phy_base_addr = 0, except for MV88E6141 and
MV88E6341 which have it as 0x10. I guess those families would have
happened to work by mistake in cross-chip scenarios too.

I'm deleting the body of mv88e6xxx_g2_irq_mdio_free() but leaving its
call sites and prototype in place. This is because, if we ever need to
add back some teardown procedure in the future, it will be perhaps
error-prone to deduce the proper call sites again. Whereas like this,
no extra code should get generated, it shouldn't bother anybody.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Klaus Kudielka <klaus.kudielka@gmail.com>
---
v3: Patch is new

 drivers/net/dsa/mv88e6xxx/global2.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)
  

This should have been [PATCH net-next v3 1/4] in the series
"net: dsa: mv88e6xxx: accelerate C45 scan".

Lore *does* recognize it as part of the series, put patchwork doesn't.
Sorry for the mistake, and please advise if I should resubmit a v4
series.

Klaus
  

On Tue, Mar 14, 2023 at 08:35:28PM +0100, Klaus Kudielka wrote:
> This should have been [PATCH net-next v3 1/4] in the series
> "net: dsa: mv88e6xxx: accelerate C45 scan".
> 
> Lore *does* recognize it as part of the series, put patchwork doesn't.
> Sorry for the mistake, and please advise if I should resubmit a v4
> series.

I'm a bit puzzled as to how you managed to get just this one patch to
have a different subject-prefix from the others?
  

On Tue, 2023-03-14 at 22:01 +0200, Vladimir Oltean wrote:
> 
> I'm a bit puzzled as to how you managed to get just this one patch to
> have a different subject-prefix from the others?

A long story, don't laugh at me.

I imported your patch with "git am", but I imported the "mbox" of the
complete message. That was the start of the disaster.

The whole E-mail was in the commit message (also the notes before the
patch), but that was easy to fix.

After git format-patch, checkpatch complained that your "From" E-mail
!= "Signed-off-by" E-mail. Obviously git has taken the "From" from the
first E-mail header.

I looked again at your patch, there it was right, and there was also
a different date (again same root cause).

So I took the shortcut: Just copy/pasted the whole patch header into
the generated patch file, without thinking further -> Boom.

(a) Don't use "git am" blindly
(b) Don't take shortcuts in the process
  

On Wed, Mar 15, 2023 at 07:07:57AM +0100, Klaus Kudielka wrote:
> On Tue, 2023-03-14 at 22:01 +0200, Vladimir Oltean wrote:
> > 
> > I'm a bit puzzled as to how you managed to get just this one patch to
> > have a different subject-prefix from the others?
> 
> A long story, don't laugh at me.
> 
> I imported your patch with "git am", but I imported the "mbox" of the
> complete message. That was the start of the disaster.
> 
> The whole E-mail was in the commit message (also the notes before the
> patch), but that was easy to fix.
> 
> After git format-patch, checkpatch complained that your "From" E-mail
> != "Signed-off-by" E-mail. Obviously git has taken the "From" from the
> first E-mail header.
> 
> I looked again at your patch, there it was right, and there was also
> a different date (again same root cause).
> 
> So I took the shortcut: Just copy/pasted the whole patch header into
> the generated patch file, without thinking further -> Boom.
> 
> (a) Don't use "git am" blindly
> (b) Don't take shortcuts in the process

Ok, so you need to go through the submission process again, to get it right.
We don't want to accept patches which were edited in-place for anything
other than the change log (the portion between "---" and the short
diffstat, which gets discarded by git anyway). The patches that are
accepted should exactly match the patches from your working git tree.
Also, netdev maintainers extremely rarely edit the patches that they
apply, to avoid introducing traceability issues.
  

On Wed, Mar 15, 2023 at 07:07:57AM +0100, Klaus Kudielka wrote:
> On Tue, 2023-03-14 at 22:01 +0200, Vladimir Oltean wrote:
> > 
> > I'm a bit puzzled as to how you managed to get just this one patch to
> > have a different subject-prefix from the others?
> 
> A long story, don't laugh at me.
> 
> I imported your patch with "git am", but I imported the "mbox" of the
> complete message. That was the start of the disaster.

What i found useful is

b4 am [msgid]

It gives you an mbox file containing just patches, which should then
cleanly git am.

	Andrew