On Thu, Aug 28, 2025 at 03:20:04PM +0200, Andrew Lunn wrote:
> > +/**
> > + * rnpgbe_reset_hw_ops - Do a hardware reset
> > + * @hw: hw information structure
> > + *
> > + * rnpgbe_reset_hw_ops calls fw to do a hardware
> > + * reset, and cleans some regs to default.
> > + *
> > + * Return: 0 on success, negative errno on failure
> > + **/
> > +static int rnpgbe_reset_hw_ops(struct mucse_hw *hw)
> > +{
> > + struct mucse_dma_info *dma = &hw->dma;
> > + int err;
> > +
> > + rnpgbe_dma_wr32(dma, RNPGBE_DMA_AXI_EN, 0);
> > + err = mucse_mbx_fw_reset_phy(hw);
> > + if (err)
> > + return err;
> > + return rnpgbe_get_permanent_mac(hw, hw->perm_addr);
>
> Why is a function named rnpgbe_reset_hw_ops() getting the permanent
> MAC address? Reset should not have anything to do with MAC address.
>
> If the MAC address is not valid, you normally use a random MAC
> address. But you cannot easily differentiate between the reset failed,
> the reset got ^C, and the MAC address was not valid.
>
Ok, I will remove call 'rnpgbe_get_permanent_mac' in function
'rnpgbe_reset_hw_ops'. And call it in probe.
> > +/**
> > + * rnpgbe_driver_status_hw_ops - Echo driver status to hw
> > + * @hw: hw information structure
> > + * @enable: true or false status
> > + * @mode: status mode
> > + **/
> > +static void rnpgbe_driver_status_hw_ops(struct mucse_hw *hw,
> > + bool enable,
> > + int mode)
> > +{
> > + switch (mode) {
> > + case mucse_driver_insmod:
> > + mucse_mbx_ifinsmod(hw, enable);
>
> Back to the ^C handling. This could be interrupted before the firmware
> is told the driver is loaded. That EINTR is thrown away here, so the
> driver thinks everything is O.K, but the firmware still thinks there
> is no MAC driver. What happens then?
>
The performance will be very poor since low working frequency,
that is not we want.
> And this is the same problem i pointed out before, you ignore EINTR in
> a void function. Rather than fix one instance, you should of reviewed
> the whole driver and fixed them all. You cannot expect the Reviewers
> to do this for you.
I see, I will change 'void' to 'int' in order to handle err, and try to check
other functions.
>
> Andrew
>
> ---
> pw-bot: cr
>
Thanks for your feedback.
