On Tue, Aug 12, 2025 at 04:49:33PM +0100, Vadim Fedorenko wrote:
> > +struct mucse_dma_info {
> > + void __iomem *dma_base_addr;
> > + void __iomem *dma_ring_addr;
> > + void *back;
>
> it might be better to keep the type of back pointer and give it
> a bit more meaningful name ...
>
> > + u32 dma_version;
> > +};
> > +
> > +struct mucse_eth_info {
> > + void __iomem *eth_base_addr;
> > + void *back;
>
> .. here ...
>
> > +};
> > +
> > +struct mucse_mac_info {
> > + void __iomem *mac_addr;
> > + void *back;
>
> and here...
>
> > +};
> > +
> > +struct mucse_mbx_info {
> > + /* fw <--> pf mbx */
> > + u32 fw_pf_shm_base;
> > + u32 pf2fw_mbox_ctrl;
> > + u32 pf2fw_mbox_mask;
> > + u32 fw_pf_mbox_mask;
> > + u32 fw2pf_mbox_vec;
> > +};
> > +
> > +struct mucse_hw {
> > + void *back;
>
> you can also use container_of() as all these structures are embedded and
> simple pointer math can give you proper result.
>
Got it, I will use container_of(), and remove the '*back' define.
Maybe eth to hw like this:
#define eth_to_hw(eth) container_of(eth, struct rnpgbe_hw, eth)
It is ok?
> > + void __iomem *hw_addr;
> > + void __iomem *ring_msix_base;
> > + struct pci_dev *pdev;
> > + enum rnpgbe_hw_type hw_type;
> > + struct mucse_dma_info dma;
> > + struct mucse_eth_info eth;
> > + struct mucse_mac_info mac;
> > + struct mucse_mbx_info mbx;
> > + u32 driver_version;
> > + u16 usecstocount;
> > +};
> > +
> > struct mucse {
> > struct net_device *netdev;
> > struct pci_dev *pdev;
> > + struct mucse_hw hw;
> > u16 bd_number;
> > };
>
> [...]
>
> > +/**
> > + * rnpgbe_add_adapter - Add netdev for this pci_dev
> > + * @pdev: PCI device information structure
> > + * @info: chip info structure
> > + *
> > + * rnpgbe_add_adapter initializes a netdev for this pci_dev
> > + * structure. Initializes Bar map, private structure, and a
> > + * hardware reset occur.
> > + *
> > + * @return: 0 on success, negative on failure
> > + **/
> > +static int rnpgbe_add_adapter(struct pci_dev *pdev,
> > + const struct rnpgbe_info *info)
> > +{
> > + struct net_device *netdev;
> > + void __iomem *hw_addr;
> > + static int bd_number;
>
> it's not clear from the patchset why do you need this static variable...
>
Ok, bd_number seems no usefull, I will remove it.
> > + struct mucse *mucse;
> > + struct mucse_hw *hw;
> > + u32 dma_version = 0;
> > + u32 queues;
> > + int err;
> > +
> > + queues = info->total_queue_pair_cnts;
> > + netdev = alloc_etherdev_mq(sizeof(struct mucse), queues);
> > + if (!netdev)
> > + return -ENOMEM;
> > +
> > + SET_NETDEV_DEV(netdev, &pdev->dev);
> > + mucse = netdev_priv(netdev);
> > + mucse->netdev = netdev;
> > + mucse->pdev = pdev;
> > + mucse->bd_number = bd_number++;
>
> ... but this code is racy by design
>
> > + pci_set_drvdata(pdev, mucse);
> > +
> > + hw = &mucse->hw;
> > + hw->back = mucse;
> > + hw->hw_type = info->hw_type;
> > + hw->pdev = pdev;
> > +
>
Thanks for your feedback.
