Re: [PATCH v3] media: saa7164: add missing ioremap error handling

Markus Elfring <[email protected]>
Newsgroups org.kernel.vger.kernel-janitors,org.kernel.vger.linux-kernel,org.kernel.vger.linux-media
Message-ID <[email protected]>
> Add checks for ioremap return values in saa7164_dev_setup(). If
> ioremap for BAR0 or BAR2 fails, release the already allocated PCI
> memory regions, remove the device from the global list, decrement
> the device count, and return -ENODEV.

See also once more:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.0-rc3#n659


> This prevents potential null pointer dereferences and ensures proper
> cleanup on memory mapping failures.

How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.0-rc3#n145


…
> +++ b/drivers/media/pci/saa7164/saa7164-core.c
> @@ -998,9 +998,21 @@ static int saa7164_dev_setup(struct saa7164_dev *dev)
>  	/* PCI/e allocations */
>  	dev->lmmio = ioremap(pci_resource_start(dev->pci, 0),
>  			     pci_resource_len(dev->pci, 0));
> +	if (!dev->lmmio) {
> +		dev_err(&dev->pci->dev,
> +				"failed to remap MMIO memory @ 0x%llx\n",
> +			(u64)pci_resource_start(dev->pci, 0));
> +		goto err_ioremap;
> +	}
>  
>  	dev->lmmio2 = ioremap(pci_resource_start(dev->pci, 2),
>  			     pci_resource_len(dev->pci, 2));
…

Would you like to avoid duplicate source code here?

…
> @@ -1019,6 +1031,23 @@ static int saa7164_dev_setup(struct saa7164_dev *dev)
>  	saa7164_pci_quirks(dev);
>  
>  	return 0;
…
> +	/* Remove from device list and decrement count */
> +	mutex_lock(&devlist);
> +	list_del(&dev->devlist);
> +	mutex_unlock(&devlist);
> +	saa7164_devcount--;
…

Will development interests grow to apply a call like “scoped_guard(mutex, &devlist)”?
https://elixir.bootlin.com/linux/v7.0-rc3/source/include/linux/mutex.h#L253

Regards,
Markus
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.