[PATCH 6/6] serial: add amba-pl011-pci

[linux@arm.linux.org.uk (Russell King - ARM Linux)]

On Fri, May 25, 2012 at 05:48:57PM +0200, Alessandro Rubini wrote:
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 070b442..e5e5ef6 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -39,14 +39,22 @@ config SERIAL_AMBA_PL010_CONSOLE
>  config SERIAL_AMBA_PL011
>  	tristate "ARM AMBA PL011 serial port support"
>  	depends on ARM_AMBA
> +	default y if STA2X11

I don't think we want to encourage an ever growing list of platforms
here.  If we did this on ARM, this would be hellishly long.

> +/* This is a template, copied every time a new pci device appears */
> +static AMBA_APB_DEVICE(pl011_pci_template, "pl011-pci", 0,
> +		       0 /* base */, {0} /* irqs */, NULL /* data */);
> +
> +static int __devinit pl011_pci_probe(struct pci_dev *pdev,
> +				     const struct pci_device_id *id)
> +{
> +	struct amba_device *adev;
> +	struct amba_pl011_data *data;
> +	int ret;
> +
> +	/* Simply build an amba device and register it */
> +	adev = kmemdup(&pl011_pci_template_device,
> +			 sizeof(pl011_pci_template_device), GFP_KERNEL);

NAK.  We have interfaces in the AMBA code for dynamically allocating
AMBA devices now - please use them instead of coding your own.  They
avoid bugs.

> +	/* change name */
> +	adev->dev.init_name = kasprintf(GFP_ATOMIC, "pl011-pci-%02x:%04x",
> +				       pdev->bus->number, pdev->devfn);
> +
> +	printk(KERN_INFO "%s %i\n", __func__, __LINE__);

This looks like debugging.

> +	if ((ret = amba_device_register(adev, &pdev->resource[0])) < 0) {
> +		kfree(adev);
> +		return ret;
> +	}
> +	return 0;
> +};
> +
> +static void __devexit pl011_pci_remove(struct pci_dev *pdev)
> +{
> +	struct amba_device *adev = pci_get_drvdata(pdev);
> +	amba_device_unregister(adev);
> +	kfree(adev->dev.platform_data);
> +	kfree(adev);

This comes nowhere close to being sane with the driver model.  This is
an oops waiting to happen.  You can't guarantee that 'adev' will be
unused by the time amba_device_unregister() (or in fact any such call
into the driver model) returns.

This is because these suckers (and all devices) are refcounted.  If you
use the correct interfaces, the devices will be freed for you automatically
at the correct time.

The only thing that won't be is adev->dev.platform_data - the correct way
to do this is the following along with the correct allocation interfaces:

	struct amba_device *adev = pci_get_drvdata(pdev);
	void *priv = adev->dev.platform_data;

	amba_device_unregister(adev);
	kfree(priv);

In other words, save off the platform data pointer, unregister the struct
device, and then free the platform data (it will not be used at that point.)