* drivers/net/phy/dp83867.c:167: possible bad if ?
@ 2015-07-20 12:37 David Binderman
2015-07-20 17:28 ` Florian Fainelli
0 siblings, 1 reply; 6+ messages in thread
From: David Binderman @ 2015-07-20 12:37 UTC (permalink / raw)
To: f.fainelli@gmail.com, netdev@vger.kernel.org
Hello there,
drivers/net/phy/dp83867.c:167:57: warning: logical ‘or’ of collectively exhaustive tests is always true [-Wlogical-op]
Source code is
if ((phydev->interface>= PHY_INTERFACE_MODE_RGMII_ID) ||
(phydev->interface <= PHY_INTERFACE_MODE_RGMII_RXID)) {
Maybe
if ((phydev->interface>= PHY_INTERFACE_MODE_RGMII_ID) &&
(phydev->interface <= PHY_INTERFACE_MODE_RGMII_RXID)) {
Regards
David Binderman
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: drivers/net/phy/dp83867.c:167: possible bad if ?
2015-07-20 12:37 drivers/net/phy/dp83867.c:167: possible bad if ? David Binderman
@ 2015-07-20 17:28 ` Florian Fainelli
2015-07-20 17:37 ` Dan Murphy
0 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2015-07-20 17:28 UTC (permalink / raw)
To: David Binderman, netdev@vger.kernel.org, dmurphy
Adding Dan,
On 20/07/15 05:37, David Binderman wrote:
> Hello there,
>
> drivers/net/phy/dp83867.c:167:57: warning: logical ‘or’ of collectively exhaustive tests is always true [-Wlogical-op]
>
> Source code is
>
> if ((phydev->interface>= PHY_INTERFACE_MODE_RGMII_ID) ||
> (phydev->interface <= PHY_INTERFACE_MODE_RGMII_RXID)) {
>
> Maybe
>
> if ((phydev->interface>= PHY_INTERFACE_MODE_RGMII_ID) &&
> (phydev->interface <= PHY_INTERFACE_MODE_RGMII_RXID)) {
Sounds like the former is the intended comparison that will make sure
that phydev->interface is between MODE_RGMII_ID and MODE_RGMII_RXID, and
not below or after.
--
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: drivers/net/phy/dp83867.c:167: possible bad if ?
2015-07-20 17:28 ` Florian Fainelli
@ 2015-07-20 17:37 ` Dan Murphy
2015-07-20 17:40 ` Florian Fainelli
0 siblings, 1 reply; 6+ messages in thread
From: Dan Murphy @ 2015-07-20 17:37 UTC (permalink / raw)
To: Florian Fainelli, David Binderman, netdev@vger.kernel.org
Florian
On 07/20/2015 12:28 PM, Florian Fainelli wrote:
> Adding Dan,
>
> On 20/07/15 05:37, David Binderman wrote:
>> Hello there,
>>
>> drivers/net/phy/dp83867.c:167:57: warning: logical ‘or’ of collectively exhaustive tests is always true [-Wlogical-op]
>>
>> Source code is
>>
>> if ((phydev->interface>= PHY_INTERFACE_MODE_RGMII_ID) ||
>> (phydev->interface <= PHY_INTERFACE_MODE_RGMII_RXID)) {
>>
>> Maybe
>>
>> if ((phydev->interface>= PHY_INTERFACE_MODE_RGMII_ID) &&
>> (phydev->interface <= PHY_INTERFACE_MODE_RGMII_RXID)) {
> Sounds like the former is the intended comparison that will make sure
> that phydev->interface is between MODE_RGMII_ID and MODE_RGMII_RXID, and
> not below or after.
That is correct. The internal delay only needs to be set if this is declared
via the DT. There can be one of 3 interface internal delay (RGMII_ID), RX internal delay (RGMII_RX_ID) or TX
internal delay (RGMII_TX_ID).
This internal delay is only applicable for RGMII and can be made specific to the
board.
The driver only needs to set the delay per the declaration in the DT.
Dan
--
------------------
Dan Murphy
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: drivers/net/phy/dp83867.c:167: possible bad if ?
2015-07-20 17:37 ` Dan Murphy
@ 2015-07-20 17:40 ` Florian Fainelli
2015-07-20 17:52 ` Dan Murphy
0 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2015-07-20 17:40 UTC (permalink / raw)
To: Dan Murphy, David Binderman, netdev@vger.kernel.org
On 20/07/15 10:37, Dan Murphy wrote:
> Florian
>
> On 07/20/2015 12:28 PM, Florian Fainelli wrote:
>> Adding Dan,
>>
>> On 20/07/15 05:37, David Binderman wrote:
>>> Hello there,
>>>
>>> drivers/net/phy/dp83867.c:167:57: warning: logical ‘or’ of collectively exhaustive tests is always true [-Wlogical-op]
>>>
>>> Source code is
>>>
>>> if ((phydev->interface>= PHY_INTERFACE_MODE_RGMII_ID) ||
>>> (phydev->interface <= PHY_INTERFACE_MODE_RGMII_RXID)) {
>>>
>>> Maybe
>>>
>>> if ((phydev->interface>= PHY_INTERFACE_MODE_RGMII_ID) &&
>>> (phydev->interface <= PHY_INTERFACE_MODE_RGMII_RXID)) {
>> Sounds like the former is the intended comparison that will make sure
>> that phydev->interface is between MODE_RGMII_ID and MODE_RGMII_RXID, and
>> not below or after.
> That is correct. The internal delay only needs to be set if this is declared
> via the DT. There can be one of 3 interface internal delay (RGMII_ID), RX internal delay (RGMII_RX_ID) or TX
> internal delay (RGMII_TX_ID).
Sorry, I meant to write the latter instead of the former here, the
current code seems to be potentially too permissive, is not it? In that
case, it seems to me like David's proposed fix is relevant.
>
> This internal delay is only applicable for RGMII and can be made specific to the
> board.
>
> The driver only needs to set the delay per the declaration in the DT.
>
> Dan
>
--
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: drivers/net/phy/dp83867.c:167: possible bad if ?
2015-07-20 17:40 ` Florian Fainelli
@ 2015-07-20 17:52 ` Dan Murphy
2015-07-20 18:00 ` David Binderman
0 siblings, 1 reply; 6+ messages in thread
From: Dan Murphy @ 2015-07-20 17:52 UTC (permalink / raw)
To: Florian Fainelli, David Binderman, netdev@vger.kernel.org
On 07/20/2015 12:40 PM, Florian Fainelli wrote:
> On 20/07/15 10:37, Dan Murphy wrote:
>> Florian
>>
>> On 07/20/2015 12:28 PM, Florian Fainelli wrote:
>>> Adding Dan,
>>>
>>> On 20/07/15 05:37, David Binderman wrote:
>>>> Hello there,
>>>>
>>>> drivers/net/phy/dp83867.c:167:57: warning: logical ‘or’ of collectively exhaustive tests is always true [-Wlogical-op]
>>>>
>>>> Source code is
>>>>
>>>> if ((phydev->interface>= PHY_INTERFACE_MODE_RGMII_ID) ||
>>>> (phydev->interface <= PHY_INTERFACE_MODE_RGMII_RXID)) {
>>>>
>>>> Maybe
>>>>
>>>> if ((phydev->interface>= PHY_INTERFACE_MODE_RGMII_ID) &&
>>>> (phydev->interface <= PHY_INTERFACE_MODE_RGMII_RXID)) {
>>> Sounds like the former is the intended comparison that will make sure
>>> that phydev->interface is between MODE_RGMII_ID and MODE_RGMII_RXID, and
>>> not below or after.
>> That is correct. The internal delay only needs to be set if this is declared
>> via the DT. There can be one of 3 interface internal delay (RGMII_ID), RX internal delay (RGMII_RX_ID) or TX
>> internal delay (RGMII_TX_ID).
> Sorry, I meant to write the latter instead of the former here, the
> current code seems to be potentially too permissive, is not it? In that
> case, it seems to me like David's proposed fix is relevant.
The proposed fix is logically correct. And it matches the code for
phy_interface_is_rgmii with the '&&'.
Did you want me to patch and test?
Dan
>
>> This internal delay is only applicable for RGMII and can be made specific to the
>> board.
>>
>> The driver only needs to set the delay per the declaration in the DT.
>>
>> Dan
>>
>
--
------------------
Dan Murphy
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: drivers/net/phy/dp83867.c:167: possible bad if ?
2015-07-20 17:52 ` Dan Murphy
@ 2015-07-20 18:00 ` David Binderman
0 siblings, 0 replies; 6+ messages in thread
From: David Binderman @ 2015-07-20 18:00 UTC (permalink / raw)
To: Dan Murphy, Florian Fainelli, netdev@vger.kernel.org
Hello there,
----------------------------------------
> The proposed fix is logically correct. And it matches the code for
> phy_interface_is_rgmii with the '&&'.
As the original warning message indicates, I used compiler flag
-Wlogical-op to find this problem.
In order to detect this problem in future in your section of
the Linux kernel, it might be a wise idea to add compiler flag
-Wlogical-op to your builds.
If all goes well, this extra compiler flag should have nothing to say.
Just an idea.
Regards
David Binderman
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-07-20 18:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-20 12:37 drivers/net/phy/dp83867.c:167: possible bad if ? David Binderman
2015-07-20 17:28 ` Florian Fainelli
2015-07-20 17:37 ` Dan Murphy
2015-07-20 17:40 ` Florian Fainelli
2015-07-20 17:52 ` Dan Murphy
2015-07-20 18:00 ` David Binderman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.