Re: [RFC PATCH v1 4/4] sample: rust: add Asix PHY driver

[FUJITA Tomonori <fujita.tomonori@gmail.com>]

Hi,

On Sun, 17 Sep 2023 17:46:39 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> >> +    fn read_status(dev: &mut phy::Device) -> Result {
>> >> +        dev.update_link()?;
>> >> +        if !dev.link() {
>> >> +            return Ok(());
>> >> +        }
>> >> +        let ret = dev.read(MII_BMCR)?;
>> >> +
>> >> +        if ret as u32 & BMCR_SPEED100 != 0 {
>> > 
>> > PHY registers are u16. phy_read() returns an int, so it can have
>> > negative return codes. But if it is not negative, the positive values
>> > are in the range of 0 to 65535.

Sorry, I should have commented in the previous reply. ret can't be a
negative here due to how to hanle an error in Rust.

let ret = dev.read(MII_BMCR)?;

Here '?' means that if an error happens, return with an error
immediately. So the ret is set only if dev.read() is sucessful.


>> A negative is an error?
>
> phy_read() can return values like -ETIMEDOUT, -ENODEV, -EIO etc to
> indicate something when wrong. So you should see the general pattern
> of:
> 
>         foo = phy_read(phydev, MII_M1111_PHY_EXT_SR);
>         if (foo < 0)
>                 return foo;
> 
> After probe, the probability of an error is very low, and generally
> there is no recovery. So not all drivers do check the return value.
> 
> There is also a second C problem here. Because a PHY register is a
> u16, you sometimes see code like:
> 
> 	u16 reg
> 
> 	reg = phy_read(phydev, MII_M1111_PHY_EXT_SR);
> 	if (reg < 0)
> 		return reg;
> 
> which compiles cleanly, but is wrong.. The folks running static
> checkers then report the bug.
> 
>> If so, this function should be:
>> 
>> fn read_status(dev: &mut phy::Device) -> Result<u16>
> 
> No, it needs to return the negative error code, or 0 on success. This
> is a very typical pattern in Linux.

This is a way to handle an error in Rust. If a function could return
an error, it returns a Result type, which is either success or
failure. Either could carry an value.

Result<u16> means if success, it carrys u16. If failure, in Rust for
Linux, carrys an error code from C side.


>> >> +    fn suspend(dev: &mut phy::Device) -> Result {
>> >> +        dev.suspend()
>> >> +    }
>> >> +
>> >> +    fn resume(dev: &mut phy::Device) -> Result {
>> >> +        dev.resume()
>> >> +    }
>> > 
>> > Can we avoid this boilerplate? A lot of phy drivers will make use of
>> > genphy_foo calls in their struct phy_driver. It would be annoying if
>> > each driver needs silly little wrappers like this.
>> 
>> you meant that C style like
>> 
>> {
>>         .suspend        = genphy_suspend,
>>         .resume         = genphy_resume,
>>         .soft_reset     = asix_soft_reset,
>> }
>> 
>> is clear than Rust style
>> 
>>     fn suspend(dev: &mut phy::Device) -> Result {
>>         dev.genphy_suspend()
>>     }
>> 
>>     fn resume(dev: &mut phy::Device) -> Result {
>>         dev.genphy_resume()
>>     }
>> 
>> right?
> 
> Yes. If you really must have these wrappers, they are not specific to
> a driver, so should be placed somewhere common where any driver can
> use them. There are around 60 of these genphy_ functions. Because what
> a PHY does is mostly standardised in IEEE 802.3, PHY drivers heavily
> use these functions to do the core of the work, and then deal with
> vendor extensions and vendor bugs.

Understood. I think that there are other places like this in the
kernel, operation function pointers that could use generic functions.

Rust people, Any opinions on a different way than vtable macro?