| Message ID | 20240923153427.2135263-1-kory.maincent@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: pse-pd: tps23881: Fix boolean evaluation for bitmask checks | expand |
On Mon, Sep 23, 2024 at 05:34:26PM +0200, Kory Maincent wrote: > Fixed potential incorrect boolean evaluation when checking bitmask values. > The existing code assigned the result of bitwise operations directly to > boolean variables, which could lead to unexpected values. > This has been corrected by explicitly converting the results to booleans > using the !! operator. > > Fixes: 20e6d190ffe1 ("net: pse-pd: Add TI TPS23881 PSE controller driver") > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> Acked-by: Oleksij Rempel <o.rempel@pengutronix.de> Thank you!
On Mon, Sep 23, 2024 at 05:34:26PM +0200, Kory Maincent wrote: > Fixed potential incorrect boolean evaluation when checking bitmask values. > The existing code assigned the result of bitwise operations directly to > boolean variables, which could lead to unexpected values. > This has been corrected by explicitly converting the results to booleans > using the !! operator. > > Fixes: 20e6d190ffe1 ("net: pse-pd: Add TI TPS23881 PSE controller driver") > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> Thanks Kory, I agree that these changes are correct. But are they fixes; can this manifest in a bug? (If so, I suspect the Kernel is riddled with such bugs.) ...
On Tue, 24 Sep 2024 08:18:39 +0100 Simon Horman <horms@kernel.org> wrote: > On Mon, Sep 23, 2024 at 05:34:26PM +0200, Kory Maincent wrote: > [...] > > Thanks Kory, > > I agree that these changes are correct. > But are they fixes; can this manifest in a bug? I didn't face it but I think yes. In case of a 4 pairs PoE ports without the fix: chan = priv->port[id].chan[0]; if (chan < 4) { enabled = ret & BIT(chan); delivering = ret & BIT(chan + 4); ... } if (priv->port[id].is_4p) { chan = priv->port[id].chan[1]; if (chan < 4) { enabled &= !!(ret & BIT(chan)); delivering &= !!(ret & BIT(chan + 4)); If enabled = 0x2 here, enabled would be assigned to 0 instead of 1. ... } } I have an issue using 4pairs PoE port with my board so I can't test it. > (If so, I suspect the Kernel is riddled with such bugs.) Don't know about it but if I can remove it from my driver it would be nice. :) Regards,
On Tue, Sep 24, 2024 at 10:15:29AM +0200, Kory Maincent wrote: > On Tue, 24 Sep 2024 08:18:39 +0100 > Simon Horman <horms@kernel.org> wrote: > > > On Mon, Sep 23, 2024 at 05:34:26PM +0200, Kory Maincent wrote: > > [...] > > > > Thanks Kory, > > > > I agree that these changes are correct. > > But are they fixes; can this manifest in a bug? > > I didn't face it but I think yes. > In case of a 4 pairs PoE ports without the fix: > > chan = priv->port[id].chan[0]; > if (chan < 4) { > enabled = ret & BIT(chan); > delivering = ret & BIT(chan + 4); > ... > } > > if (priv->port[id].is_4p) { > chan = priv->port[id].chan[1]; > if (chan < 4) { > enabled &= !!(ret & BIT(chan)); > delivering &= !!(ret & BIT(chan + 4)); > > If enabled = 0x2 here, enabled would be assigned to 0 instead of 1. > ... > > } > } > > I have an issue using 4pairs PoE port with my board so I can't test it. > > > > (If so, I suspect the Kernel is riddled with such bugs.) > > Don't know about it but if I can remove it from my driver it would be nice. :) Right, no question from my side that this change is a good one. I'm just wondering if it is best for net or net-next.
On Tue, 24 Sep 2024 09:26:12 +0100 Simon Horman <horms@kernel.org> wrote: > > Don't know about it but if I can remove it from my driver it would be nice. > > :) > > Right, no question from my side that this change is a good one. > I'm just wondering if it is best for net or net-next. Indeed, I don't know the policy on this. Do you think it shouldn't go to net? I will let net maintainers decide. ;) Regards,
On Tue, Sep 24, 2024 at 10:33:57AM +0200, Kory Maincent wrote: > On Tue, 24 Sep 2024 09:26:12 +0100 > Simon Horman <horms@kernel.org> wrote: > > > > Don't know about it but if I can remove it from my driver it would be nice. > > > :) > > > > Right, no question from my side that this change is a good one. > > I'm just wondering if it is best for net or net-next. > > Indeed, I don't know the policy on this. Do you think it shouldn't go to net? > I will let net maintainers decide. ;) The net is always the right place for this kind of fixes. In any case, it would be better to have actual symptoms of the issue addressed by this patch in the commit message.
On Tue, Sep 24, 2024 at 10:44:06AM +0200, Oleksij Rempel wrote: > On Tue, Sep 24, 2024 at 10:33:57AM +0200, Kory Maincent wrote: > > On Tue, 24 Sep 2024 09:26:12 +0100 > > Simon Horman <horms@kernel.org> wrote: > > > > > > Don't know about it but if I can remove it from my driver it would be nice. > > > > :) > > > > > > Right, no question from my side that this change is a good one. > > > I'm just wondering if it is best for net or net-next. > > > > Indeed, I don't know the policy on this. Do you think it shouldn't go to net? > > I will let net maintainers decide. ;) > > The net is always the right place for this kind of fixes. In any case, it > would be better to have actual symptoms of the issue addressed by > this patch in the commit message. Thanks, I think that would address my questions about this patch.
diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c index 5c4e88be46ee..1a57c55f8577 100644 --- a/drivers/net/pse-pd/tps23881.c +++ b/drivers/net/pse-pd/tps23881.c @@ -139,9 +139,9 @@ static int tps23881_pi_is_enabled(struct pse_controller_dev *pcdev, int id) chan = priv->port[id].chan[0]; if (chan < 4) - enabled = ret & BIT(chan); + enabled = !!(ret & BIT(chan)); else - enabled = ret & BIT(chan + 4); + enabled = !!(ret & BIT(chan + 4)); if (priv->port[id].is_4p) { chan = priv->port[id].chan[1]; @@ -172,11 +172,11 @@ static int tps23881_ethtool_get_status(struct pse_controller_dev *pcdev, chan = priv->port[id].chan[0]; if (chan < 4) { - enabled = ret & BIT(chan); - delivering = ret & BIT(chan + 4); + enabled = !!(ret & BIT(chan)); + delivering = !!(ret & BIT(chan + 4)); } else { - enabled = ret & BIT(chan + 4); - delivering = ret & BIT(chan + 8); + enabled = !!(ret & BIT(chan + 4)); + delivering = !!(ret & BIT(chan + 8)); } if (priv->port[id].is_4p) {
Fixed potential incorrect boolean evaluation when checking bitmask values. The existing code assigned the result of bitwise operations directly to boolean variables, which could lead to unexpected values. This has been corrected by explicitly converting the results to booleans using the !! operator. Fixes: 20e6d190ffe1 ("net: pse-pd: Add TI TPS23881 PSE controller driver") Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> --- drivers/net/pse-pd/tps23881.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)