Re: [PATCH v2 02/15] net: rnpgbe: Add n500/n210 chip support

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 22/07/2025 10:51, Yibo Dong wrote:
> On Mon, Jul 21, 2025 at 03:21:23PM +0100, Vadim Fedorenko wrote:
>> On 21/07/2025 12:32, Dong Yibo wrote:
>>> Initialize n500/n210 chip bar resource map and
>>> dma, eth, mbx ... info for future use.
>>>
>>> Signed-off-by: Dong Yibo <dong100@mucse.com>
>>> ---
>>>    drivers/net/ethernet/mucse/rnpgbe/Makefile    |   4 +-
>>>    drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    | 138 ++++++++++++++++++
>>>    .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c   | 138 ++++++++++++++++++
>>>    drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h |  27 ++++
>>>    .../net/ethernet/mucse/rnpgbe/rnpgbe_main.c   |  68 ++++++++-
>>>    5 files changed, 370 insertions(+), 5 deletions(-)
>>>    create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
>>>    create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
>>>

[...]

>>> +/**
>>> + * rnpgbe_get_invariants_n500 - setup for hw info
>>> + * @hw: hw information structure
>>> + *
>>> + * rnpgbe_get_invariants_n500 initializes all private
>>> + * structure, such as dma, eth, mac and mbx base on
>>> + * hw->addr for n500
>>> + **/
>>> +static void rnpgbe_get_invariants_n500(struct mucse_hw *hw)
>>> +{
>>> +	struct mucse_dma_info *dma = &hw->dma;
>>> +	struct mucse_eth_info *eth = &hw->eth;
>>> +	struct mucse_mac_info *mac = &hw->mac;
>>> +	struct mucse_mbx_info *mbx = &hw->mbx;
>>> +
>>> +	/* setup msix base */
>>> +	hw->ring_msix_base = hw->hw_addr + 0x28700;
>>> +	/* setup dma info */
>>> +	dma->dma_base_addr = hw->hw_addr;
>>> +	dma->dma_ring_addr = hw->hw_addr + RNPGBE_RING_BASE;
>>> +	dma->max_tx_queues = RNPGBE_MAX_QUEUES;
>>> +	dma->max_rx_queues = RNPGBE_MAX_QUEUES;
>>> +	dma->back = hw;
>>> +	/* setup eth info */
>>> +	eth->eth_base_addr = hw->hw_addr + RNPGBE_ETH_BASE;
>>> +	eth->back = hw;
>>> +	eth->mc_filter_type = 0;
>>> +	eth->mcft_size = RNPGBE_MC_TBL_SIZE;
>>> +	eth->vft_size = RNPGBE_VFT_TBL_SIZE;
>>> +	eth->num_rar_entries = RNPGBE_RAR_ENTRIES;
>>> +	/* setup mac info */
>>> +	mac->mac_addr = hw->hw_addr + RNPGBE_MAC_BASE;
>>> +	mac->back = hw;
>>> +	/* set mac->mii */
>>> +	mac->mii.addr = RNPGBE_MII_ADDR;
>>> +	mac->mii.data = RNPGBE_MII_DATA;
>>> +	mac->mii.addr_shift = 11;
>>> +	mac->mii.addr_mask = 0x0000F800;
>>> +	mac->mii.reg_shift = 6;
>>> +	mac->mii.reg_mask = 0x000007C0;
>>> +	mac->mii.clk_csr_shift = 2;
>>> +	mac->mii.clk_csr_mask = GENMASK(5, 2);
>>> +	mac->clk_csr = 0x02; /* csr 25M */
>>> +	/* hw fixed phy_addr */
>>> +	mac->phy_addr = 0x11;
>>> +
>>> +	mbx->mbx_feature |= MBX_FEATURE_NO_ZERO;
>>> +	/* mbx offset */
>>> +	mbx->vf2pf_mbox_vec_base = 0x28900;
>>> +	mbx->fw2pf_mbox_vec = 0x28b00;
>>> +	mbx->pf_vf_shm_base = 0x29000;
>>> +	mbx->mbx_mem_size = 64;
>>> +	mbx->pf2vf_mbox_ctrl_base = 0x2a100;
>>> +	mbx->pf_vf_mbox_mask_lo = 0x2a200;
>>> +	mbx->pf_vf_mbox_mask_hi = 0;
>>> +	mbx->fw_pf_shm_base = 0x2d000;
>>> +	mbx->pf2fw_mbox_ctrl = 0x2e000;
>>> +	mbx->fw_pf_mbox_mask = 0x2e200;
>>> +	mbx->fw_vf_share_ram = 0x2b000;
>>> +	mbx->share_size = 512;
>>> +
>>> +	/* setup net feature here */
>>> +	hw->feature_flags |= M_NET_FEATURE_SG |
>>> +			     M_NET_FEATURE_TX_CHECKSUM |
>>> +			     M_NET_FEATURE_RX_CHECKSUM |
>>> +			     M_NET_FEATURE_TSO |
>>> +			     M_NET_FEATURE_VLAN_FILTER |
>>> +			     M_NET_FEATURE_VLAN_OFFLOAD |
>>> +			     M_NET_FEATURE_RX_NTUPLE_FILTER |
>>> +			     M_NET_FEATURE_RX_HASH |
>>> +			     M_NET_FEATURE_USO |
>>> +			     M_NET_FEATURE_RX_FCS |
>>> +			     M_NET_FEATURE_STAG_FILTER |
>>> +			     M_NET_FEATURE_STAG_OFFLOAD;
>>> +	/* start the default ahz, update later */
>>> +	hw->usecstocount = 125;
>>> +}
>>> +
>>> +/**
>>> + * rnpgbe_get_invariants_n210 - setup for hw info
>>> + * @hw: hw information structure
>>> + *
>>> + * rnpgbe_get_invariants_n210 initializes all private
>>> + * structure, such as dma, eth, mac and mbx base on
>>> + * hw->addr for n210
>>> + **/
>>> +static void rnpgbe_get_invariants_n210(struct mucse_hw *hw)
>>> +{
>>> +	struct mucse_mbx_info *mbx = &hw->mbx;
>>> +	/* get invariants based from n500 */
>>> +	rnpgbe_get_invariants_n500(hw);
>>
>> it's not a good pattern. if you have some configuration that is
>> shared amoung devices, it's better to create *base() or *common()
>> helper and call it from each specific initializer. BTW, why do you
>> name these functions get_invariants*()? They don't get anything, but
>> rather init/setup configuration values. It's better to rename it
>> according to the function.
>>
> 
> I try to devide hardware to dma, eth, mac, mbx modules. Different
> chips may use the same mbx module with different reg-offset in bar.
> So I setup reg-offset in get_invariants for each chip. And common code,
> such as mbx achieve functions with the reg-offset.
> Ok, I will rename it.

I fully understand your intention. My point is that calling
rnpgbe_get_invariants_n500(hw) in rnpgbe_get_invariants_n210() and
then replace almost half of the values is not a good pattern.
It's better to have another function to setup values that are the same
across models, and keep only specifics in *n500() and *n210().

> 
>>> +
>>> +	/* update msix base */
>>> +	hw->ring_msix_base = hw->hw_addr + 0x29000;
>>> +	/* update mbx offset */
>>> +	mbx->vf2pf_mbox_vec_base = 0x29200;
>>> +	mbx->fw2pf_mbox_vec = 0x29400;
>>> +	mbx->pf_vf_shm_base = 0x29900;
>>> +	mbx->mbx_mem_size = 64;
>>> +	mbx->pf2vf_mbox_ctrl_base = 0x2aa00;
>>> +	mbx->pf_vf_mbox_mask_lo = 0x2ab00;
>>> +	mbx->pf_vf_mbox_mask_hi = 0;
>>> +	mbx->fw_pf_shm_base = 0x2d900;
>>> +	mbx->pf2fw_mbox_ctrl = 0x2e900;
>>> +	mbx->fw_pf_mbox_mask = 0x2eb00;
>>> +	mbx->fw_vf_share_ram = 0x2b900;
>>> +	mbx->share_size = 512;
>>> +	/* update hw feature */
>>> +	hw->feature_flags |= M_HW_FEATURE_EEE;
>>> +	hw->usecstocount = 62;
>>> +}

[...]

>>> @@ -58,7 +72,54 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev)
>>>    		 rnpgbe_driver_name, mucse->bd_number);
>>>    	pci_set_drvdata(pdev, mucse);
>>> +	hw = &mucse->hw;
>>> +	hw->back = mucse;
>>> +	hw->hw_type = ii->hw_type;
>>> +
>>> +	switch (hw->hw_type) {
>>> +	case rnpgbe_hw_n500:
>>> +		/* n500 use bar2 */
>>> +		hw_addr = devm_ioremap(&pdev->dev,
>>> +				       pci_resource_start(pdev, 2),
>>> +				       pci_resource_len(pdev, 2));
>>> +		if (!hw_addr) {
>>> +			dev_err(&pdev->dev, "map bar2 failed!\n");
>>> +			return -EIO;
>>> +		}
>>> +
>>> +		/* get dma version */
>>> +		dma_version = m_rd_reg(hw_addr);
>>> +		break;
>>> +	case rnpgbe_hw_n210:
>>> +	case rnpgbe_hw_n210L:
>>> +		/* check bar0 to load firmware */
>>> +		if (pci_resource_len(pdev, 0) == 0x100000)
>>> +			return -EIO;
>>> +		/* n210 use bar2 */
>>> +		hw_addr = devm_ioremap(&pdev->dev,
>>> +				       pci_resource_start(pdev, 2),
>>> +				       pci_resource_len(pdev, 2));
>>> +		if (!hw_addr) {
>>> +			dev_err(&pdev->dev, "map bar2 failed!\n");
>>> +			return -EIO;
>>> +		}
>>> +
>>> +		/* get dma version */
>>> +		dma_version = m_rd_reg(hw_addr);
>>> +		break;
>>> +	default:
>>> +		err = -EIO;
>>> +		goto err_free_net;
>>> +	}
>>> +	hw->hw_addr = hw_addr;
>>> +	hw->dma.dma_version = dma_version;
>>> +	ii->get_invariants(hw);
>>> +
>>>    	return 0;
>>> +
>>> +err_free_net:
>>> +	free_netdev(netdev);
>>> +	return err;
>>>    }
>>
>> You have err_free_net label, which is used only in really impossible
>> case of unknown device, while other cases can return directly and
>> memleak netdev...>>
> 
> Yes, It is really impossible case of unknown device. But maybe switch
> should always has 'default case'? And if in 'default case', nothing To
> do but free_netdev and return err.
> Other cases return directly with return 0, and netdev will be freed in
> rnpgbe_rm_adapter() when rmmod. Sorry, I may not have got the memleak
> point?

Both rnpgbe_hw_n500 and rnpgbe_hw_n200 cases have error paths which
directly return -EIO. In this case netdev is not freed and
rnpgbe_rm_adapter() will not happen as rnpgbe_add_adapter() didn't
succeed.


> 
>>>    /**
>>> @@ -74,6 +135,7 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev)
>>>     **/
>>>    static int rnpgbe_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>>    {
>>> +	const struct rnpgbe_info *ii = rnpgbe_info_tbl[id->driver_data];
>>>    	int err;
>>>    	err = pci_enable_device_mem(pdev);
>>> @@ -97,7 +159,7 @@ static int rnpgbe_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>>    	pci_set_master(pdev);
>>>    	pci_save_state(pdev);
>>> -	err = rnpgbe_add_adapter(pdev);
>>> +	err = rnpgbe_add_adapter(pdev, ii);
>>>    	if (err)
>>>    		goto err_regions;
>>
>>
> 
> Thanks for your feedback.
>