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(-)

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)

-- 
2.30.2

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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/

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?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

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.