Re: [PATCH v2 1/4] soc: marvell: Add a general purpose RVU PF driver

[Krzysztof Kozlowski]

On 01/10/2024 12:49, Anshumali Gaur wrote:
> Resource virtualization unit (RVU) on Marvell's Octeon series of
> silicons maps HW resources from the network, crypto and other
> functional blocks into PCI-compatible physical and virtual functions.
> Each functional block again has multiple local functions (LFs) for
> provisioning to PCI devices.
> RVU supports multiple PCIe SRIOV physical functions (PFs) and virtual
> functions (VFs). And RVU admin function (AF) is the one which manages
> all the resources (local functions etc) in the system.

I responded to you on your v1 that you should not CC people totally
irrelevant to this patchset: like me.

I gave you also extensive guide how to use tools to find people to cc.

But no, you decided to keep spamming me.

Since you insist, annoyed review follows:

...


> +
> +#define DRV_NAME    "rvu_generic_pf"
> +
> +/* Supported devices */
> +static const struct pci_device_id rvu_gen_pf_id_table[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, 0xA0F6) },
> +	{ }  /* end of table */
> +};
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Marvell Octeon RVU Generic PF Driver");
> +MODULE_DEVICE_TABLE(pci, rvu_gen_pf_id_table);

All above go to the end of the module.

> +
> +static int rvu_gen_pf_check_pf_usable(struct gen_pf_dev *pfdev)
> +{
> +	u64 rev;
> +
> +	rev = readq(pfdev->reg_base + RVU_PF_BLOCK_ADDRX_DISC(BLKADDR_RVUM));
> +	rev = (rev >> 12) & 0xFF;
> +	/* Check if AF has setup revision for RVUM block,

Use Linux coding style. See coding style document to figure out how
comment should look like.

> +	 * otherwise this driver probe should be deferred
> +	 * until AF driver comes up.
> +	 */
> +	if (!rev) {
> +		dev_warn(pfdev->dev,
> +			 "AF is not initialized, deferring probe\n");
> +		return -EPROBE_DEFER;

That's just racy...

> +	}
> +	return 0;
> +}
> +
> +static int rvu_gen_pf_sriov_enable(struct pci_dev *pdev, int numvfs)
> +{
> +	int ret;
> +
> +	ret = pci_enable_sriov(pdev, numvfs);
> +	if (ret)
> +		return ret;
> +
> +	return numvfs;
> +}
> +
> +static int rvu_gen_pf_sriov_disable(struct pci_dev *pdev)
> +{
> +	int numvfs = pci_num_vf(pdev);
> +
> +	if (!numvfs)
> +		return 0;
> +
> +	pci_disable_sriov(pdev);
> +
> +	return 0;
> +}
> +
> +static int rvu_gen_pf_sriov_configure(struct pci_dev *pdev, int numvfs)
> +{
> +	if (numvfs == 0)
> +		return rvu_gen_pf_sriov_disable(pdev);
> +
> +	return rvu_gen_pf_sriov_enable(pdev, numvfs);
> +}
> +
> +static void rvu_gen_pf_remove(struct pci_dev *pdev)
> +{
> +	struct gen_pf_dev *pfdev = pci_get_drvdata(pdev);
> +
> +	rvu_gen_pf_sriov_disable(pfdev->pdev);
> +	pci_set_drvdata(pdev, NULL);
> +
> +	pci_release_regions(pdev);
> +}
> +
> +static int rvu_gen_pf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct gen_pf_dev *pfdev;
> +	int err;
> +
> +	err = pcim_enable_device(pdev);
> +	if (err) {
> +		dev_err(dev, "Failed to enable PCI device\n");
> +		return err;
> +	}
> +
> +	err = pci_request_regions(pdev, DRV_NAME);
> +	if (err) {
> +		dev_err(dev, "PCI request regions failed %d\n", err);
> +		goto err_map_failed;
> +	}
> +
> +	err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(48));
> +	if (err) {
> +		dev_err(dev, "DMA mask config failed, abort\n");
> +		goto err_release_regions;
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	pfdev = devm_kzalloc(dev, sizeof(struct gen_pf_dev), GFP_KERNEL);

sizeof(*)


Are you sure you run checkpatch --strict on this?

Best regards,
Krzysztof