diff mbox series

[RFC,net-next,v2,03/12] net: mdio: mdiobus_register: update validation test

Message ID 20221227-v6-2-rc1-c45-seperation-v2-3-ddb37710e5a7@walle.cc (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series net: mdio: Start separating C22 and C45 | expand

Commit Message

Michael Walle Dec. 27, 2022, 11:07 p.m. UTC 
From: Andrew Lunn <andrew@lunn.ch>

Now that C45 uses its own read/write methods, the validation performed
when a bus is registers needs updating. All combinations of C22 and
C45 are supported, but both read and write methods must be provided,
read only busses are not supported etc.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Michael Walle <michael@walle.cc>
---
v2:
 - [al] be consistent with other checks
 - [mw] make the test a bit easier to read
---
 drivers/net/phy/mdio_bus.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Russell King (Oracle) Jan. 3, 2023, 10:13 a.m. UTC | #1 
Hi Michael,

Thanks for picking this up!

On Wed, Dec 28, 2022 at 12:07:19AM +0100, Michael Walle wrote:
> +	if (!bus || !bus->name)
> +		return -EINVAL;
> +
> +	/* An access method always needs both read and write operations */
> +	if ((bus->read && !bus->write) ||
> +	    (!bus->read && bus->write) ||
> +	    (bus->read_c45 && !bus->write_c45) ||
> +	    (!bus->read_c45 && bus->write_c45))

I wonder whether the following would be even more readable:

	if (!bus->read != !bus->write || !bus->read_c45 != !bus->write_c45)

which essentially asserts that the boolean of !method for the read and
write methods must match.
Michael Walle Jan. 3, 2023, 10:21 a.m. UTC | #2 
Hi Russell,

Am 2023-01-03 11:13, schrieb Russell King (Oracle):
> On Wed, Dec 28, 2022 at 12:07:19AM +0100, Michael Walle wrote:
>> +	if (!bus || !bus->name)
>> +		return -EINVAL;
>> +
>> +	/* An access method always needs both read and write operations */
>> +	if ((bus->read && !bus->write) ||
>> +	    (!bus->read && bus->write) ||
>> +	    (bus->read_c45 && !bus->write_c45) ||
>> +	    (!bus->read_c45 && bus->write_c45))
> 
> I wonder whether the following would be even more readable:
> 
> 	if (!bus->read != !bus->write || !bus->read_c45 != !bus->write_c45)

That's what Andrew had originally. But there was a comment from Sergey 
[1]
which I agree with. I had a hard time wrapping my head around that, so I
just listed all the possible bad cases.

I don't have a strong opinion, though.

> which essentially asserts that the boolean of !method for the read and
> write methods must match.

Maybe with that as a comment?

-michael

[1] 
https://lore.kernel.org/netdev/ae79823f-3697-feee-32e6-645c6f4b4e93@omp.ru/
Russell King (Oracle) Jan. 3, 2023, 10:19 p.m. UTC | #3 
Hi Michael,

On Tue, Jan 03, 2023 at 11:21:08AM +0100, Michael Walle wrote:
> Hi Russell,
> 
> Am 2023-01-03 11:13, schrieb Russell King (Oracle):
> > On Wed, Dec 28, 2022 at 12:07:19AM +0100, Michael Walle wrote:
> > > +	if (!bus || !bus->name)
> > > +		return -EINVAL;
> > > +
> > > +	/* An access method always needs both read and write operations */
> > > +	if ((bus->read && !bus->write) ||
> > > +	    (!bus->read && bus->write) ||
> > > +	    (bus->read_c45 && !bus->write_c45) ||
> > > +	    (!bus->read_c45 && bus->write_c45))
> > 
> > I wonder whether the following would be even more readable:
> > 
> > 	if (!bus->read != !bus->write || !bus->read_c45 != !bus->write_c45)
> 
> That's what Andrew had originally. But there was a comment from Sergey [1]
> which I agree with. I had a hard time wrapping my head around that, so I
> just listed all the possible bad cases.

The only reason I suggested it was because when looked at your code,
it also took several reads to work out what it was trying to do!

Would using !!bus->read != !!bus->write would help or make it worse,
!!ptr being the more normal way to convert something to a boolean?
Michael Walle Jan. 9, 2023, 12:35 p.m. UTC | #4 
Hi Russell,

Am 2023-01-03 23:19, schrieb Russell King (Oracle):
> On Tue, Jan 03, 2023 at 11:21:08AM +0100, Michael Walle wrote:
>> Am 2023-01-03 11:13, schrieb Russell King (Oracle):
>> > On Wed, Dec 28, 2022 at 12:07:19AM +0100, Michael Walle wrote:
>> > > +	if (!bus || !bus->name)
>> > > +		return -EINVAL;
>> > > +
>> > > +	/* An access method always needs both read and write operations */
>> > > +	if ((bus->read && !bus->write) ||
>> > > +	    (!bus->read && bus->write) ||
>> > > +	    (bus->read_c45 && !bus->write_c45) ||
>> > > +	    (!bus->read_c45 && bus->write_c45))
>> >
>> > I wonder whether the following would be even more readable:
>> >
>> > 	if (!bus->read != !bus->write || !bus->read_c45 != !bus->write_c45)
>> 
>> That's what Andrew had originally. But there was a comment from Sergey 
>> [1]
>> which I agree with. I had a hard time wrapping my head around that, so 
>> I
>> just listed all the possible bad cases.
> 
> The only reason I suggested it was because when looked at your code,
> it also took several reads to work out what it was trying to do!
> 
> Would using !!bus->read != !!bus->write would help or make it worse,
> !!ptr being the more normal way to convert something to a boolean?

IMHO that makes it even harder. But I doubt we will find an expression
that will work for everyone. I'll go with your suggestion/Andrew's first
version in the next iteration.

-michael
Vladimir Oltean Jan. 9, 2023, 12:41 p.m. UTC | #5 
On Mon, Jan 09, 2023 at 01:35:29PM +0100, Michael Walle wrote:
> Hi Russell,
> 
> Am 2023-01-03 23:19, schrieb Russell King (Oracle):
> > On Tue, Jan 03, 2023 at 11:21:08AM +0100, Michael Walle wrote:
> > > Am 2023-01-03 11:13, schrieb Russell King (Oracle):
> > > > On Wed, Dec 28, 2022 at 12:07:19AM +0100, Michael Walle wrote:
> > > > > +	if (!bus || !bus->name)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	/* An access method always needs both read and write operations */
> > > > > +	if ((bus->read && !bus->write) ||
> > > > > +	    (!bus->read && bus->write) ||
> > > > > +	    (bus->read_c45 && !bus->write_c45) ||
> > > > > +	    (!bus->read_c45 && bus->write_c45))
> > > >
> > > > I wonder whether the following would be even more readable:
> > > >
> > > > 	if (!bus->read != !bus->write || !bus->read_c45 != !bus->write_c45)
> > > 
> > > That's what Andrew had originally. But there was a comment from
> > > Sergey [1]
> > > which I agree with. I had a hard time wrapping my head around that,
> > > so I
> > > just listed all the possible bad cases.
> > 
> > The only reason I suggested it was because when looked at your code,
> > it also took several reads to work out what it was trying to do!
> > 
> > Would using !!bus->read != !!bus->write would help or make it worse,
> > !!ptr being the more normal way to convert something to a boolean?
> 
> IMHO that makes it even harder. But I doubt we will find an expression
> that will work for everyone. I'll go with your suggestion/Andrew's first
> version in the next iteration.

I think the double negation conveys the intention better than the simple
one, actually (maybe even xor instead of != ?). In terms of readability
I think I prefer the way the patch is written right now, but if you keep
the comment, the double negation should be pretty easy to swallow too.
diff mbox series

Patch

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index bde195864c17..d14d7704e895 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -526,8 +526,18 @@  int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 	int i, err;
 	struct gpio_desc *gpiod;
 
-	if (NULL == bus || NULL == bus->name ||
-	    NULL == bus->read || NULL == bus->write)
+	if (!bus || !bus->name)
+		return -EINVAL;
+
+	/* An access method always needs both read and write operations */
+	if ((bus->read && !bus->write) ||
+	    (!bus->read && bus->write) ||
+	    (bus->read_c45 && !bus->write_c45) ||
+	    (!bus->read_c45 && bus->write_c45))
+		return -EINVAL;
+
+	/* At least one method is mandatory */
+	if (!bus->read && !bus->read_c45)
 		return -EINVAL;
 
 	if (bus->parent && bus->parent->of_node)