> +/**
> + * 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.
> +/**
> + * 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?
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.
Andrew
---
pw-bot: cr
