From: Florian Fainelli <f.fainelli@gmail.com>
To: Rodolfo Giometti <giometti@enneenne.com>, Andrew Lunn <andrew@lunn.ch>
Cc: Vivien Didelot <vivien.didelot@savoirfairelinux.com>,
"David S. Miller" <davem@davemloft.net>,
netdev@vger.kernel.org
Subject: Re: Possible bug into DSA2 code.
Date: Mon, 11 Feb 2019 10:01:22 -0800 [thread overview]
Message-ID: <2651a882-c5ab-ba3b-ef4c-731dca90147d@gmail.com> (raw)
In-Reply-To: <49728829-49cc-6d6a-04b1-ccfc1e5266dd@enneenne.com>
On 2/11/19 9:51 AM, Rodolfo Giometti wrote:
> On 11/02/2019 18:28, Florian Fainelli wrote:
>> On 2/10/19 3:45 AM, Rodolfo Giometti wrote:
>>> On 09/02/2019 20:34, Andrew Lunn wrote:
>>>>> So we I see two possible solutions:
>>>>>
>>>>> 1) having both ds->slave_mii_bus and ds->ops->phy_read already
>>>>> defined is an
>>>>> error, then it must be signaled to the calling code, or
>>>>
>>>> I don't think we can do that. mv88e6xxx optionally instantiates the
>>>> MDIO busses, depending on what is in device tree. If there is no mdio
>>>> property, we need the DSA core to create an MDIO bus.
>>>
>>> OK, but using the following check to know if DSA did such allocation is
>>> not correct because DSA drivers can allocate it by their own:
>>>
>>> static void dsa_switch_teardown(struct dsa_switch *ds)
>>> {
>>> if (ds->slave_mii_bus && ds->ops->phy_read)
>>> mdiobus_unregister(ds->slave_mii_bus);
>>>
>>> Maybe can we add a flag to register ds->slave_mii_bus allocation by DSA?
>>
>> If drivers allocate the slave_mii_bus, or use it as a pointer to their
>> bus, then they should not be providing a ds->ops->phy_read() callback
>> since we assume they would have mii_bus::read and mii_bus::write set to
>> their driver internal version.
>
> I see, so having ds->slave_mii_bus and ds->ops->phy_read both not NULL
> into dsa_switch_setup() is a potential bug, I suppose... If so why not
> adding a BUG_ON() call to signal it instead of doing nothing? :-o
If you have both non NULL, then your driver did allocate
ds->slave_mii_bus on its own, and also assigned a valid
ds->ops->phy_read() then things will work, except that
ds->ops->phy_read() will not be used. And yes, that is going to be
blowing away when the whole DSA tree gets teardowned.
If you want to add a check for that condition, that would be a good
thing, just not a BUG_ON(), propagate an error back to the caller and
abort the tree/switch probing.
--
Florian
next prev parent reply other threads:[~2019-02-11 18:04 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <a121e6b5-03cd-da9e-42e8-41c68e12babe@enneenne.com>
2019-02-09 8:24 ` Possible bug into DSA2 code Rodolfo Giometti
2019-02-09 19:34 ` Andrew Lunn
2019-02-10 11:45 ` Rodolfo Giometti
2019-02-11 17:28 ` Florian Fainelli
2019-02-11 17:51 ` Rodolfo Giometti
2019-02-11 18:01 ` Florian Fainelli [this message]
2019-02-11 19:13 ` Florian Fainelli
2019-02-18 10:38 ` Rodolfo Giometti
2019-02-19 0:03 ` Andrew Lunn
2019-02-20 7:54 ` Rodolfo Giometti
2019-02-20 15:02 ` Andrew Lunn
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=2651a882-c5ab-ba3b-ef4c-731dca90147d@gmail.com \
--to=f.fainelli@gmail.com \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=giometti@enneenne.com \
--cc=netdev@vger.kernel.org \
--cc=vivien.didelot@savoirfairelinux.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).