Date:	Sun, 3 May 2015 15:33:40 +0200
From:	Jean Delvare <jdelvare@...e.de>
To:	Sudip Mukherjee <sudipm.mukherjee@...il.com>
Cc:	Greg KH <gregkh@...uxfoundation.org>,
	Dan Carpenter <dan.carpenter@...cle.com>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 WIP 2/4] i2c-parport: modify driver to use new
 parport device model

Hi Sudip,

On Tue, 28 Apr 2015 17:00:21 +0530, Sudip Mukherjee wrote:
> modify i2c-parport driver to use the new parallel port device model.
> 
> v4: according to the suggestion of Alan, array is being used in the
> module parameter. Hopefully no one will use more than 4 instances.
> 
> Hi Jean,
> This patchset is good for testing. Can you please check what is
> happening to your hardware...

I don't have the time to review the first patch with the core changes,
but I can comment on this second patch as a driver author.

Testing report will come a bit later.

> Signed-off-by: Sudip Mukherjee <sudip@...torindia.org>
> ---
>  drivers/i2c/busses/i2c-parport.c | 95 ++++++++++++++++++++++++++++------------
>  drivers/i2c/busses/i2c-parport.h |  7 +++
>  2 files changed, 74 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
> index a1fac5a..b07db1c 100644
> --- a/drivers/i2c/busses/i2c-parport.c
> +++ b/drivers/i2c/busses/i2c-parport.c
> @@ -160,20 +160,49 @@ static void i2c_parport_irq(void *data)
>  			"SMBus alert received but no ARA client!\n");
>  }
>  
> +static struct pardev_cb i2c_parport_cb = {
> +	.flags = PARPORT_FLAG_EXCL,
> +	.irq_func = i2c_parport_irq,
> +};
> +
>  static void i2c_parport_attach(struct parport *port)
>  {
>  	struct i2c_par *adapter;
> +	int i;
> +	char *name;
> +
> +	if (!is_parport(&port->bus_dev))
> +		return;

Can this actually happen?

> +
> +	name = kzalloc(13, GFP_KERNEL);
> +	if (!name)
> +		return;
> +
> +	for (i = 0; i < 4; i++) {

Please introduce a define for the maximum number of devices supported,
and use it everywhere this is relevant.

> +		if (parport[i] == -1)
> +			continue;
> +		if (port->number == parport[i])
> +			break;
> +	}
> +	if (i == 4) {	/* port mentioned not found */

Shouldn't this be reported as an error to the user?

> +		kfree(name);
> +		return;

Please move the kfree to the error path at the end of the function, and
use goto to jump there, as is already done in the rest of the function.
Or maybe this is not needed at all, see below.

> +	}
> +	sprintf(name, "i2c-parport%d", i);

Seems weird, see below.

>  
>  	adapter = kzalloc(sizeof(struct i2c_par), GFP_KERNEL);
>  	if (adapter == NULL) {
>  		printk(KERN_ERR "i2c-parport: Failed to kzalloc\n");
> +		kfree(name);
>  		return;
>  	}
> +	i2c_parport_cb.private = adapter;
>  
>  	pr_debug("i2c-parport: attaching to %s\n", port->name);
>  	parport_disable_irq(port);
> -	adapter->pdev = parport_register_device(port, "i2c-parport",
> -		NULL, NULL, i2c_parport_irq, PARPORT_FLAG_EXCL, adapter);
> +	adapter->pdev = parport_register_dev_model(port, name,
> +						   &i2c_parport_cb);
> +	kfree(name);

If you can free "name" at this point, this suggests that
parport_register_dev_model made a copy somehow. In that case, please
don't use dynamic memory allocation in the first place. Either use a
static buffer and sprintf to it, or (probably better) pass the instance
number to parport_register_dev_model() as a separate parameter.

>  	if (!adapter->pdev) {
>  		printk(KERN_ERR "i2c-parport: Unable to register with parport\n");
>  		goto err_free;
> @@ -237,39 +266,26 @@ static void i2c_parport_attach(struct parport *port)
>  	parport_unregister_device(adapter->pdev);
>   err_free:
>  	kfree(adapter);
> +	return;

This return statement serves no purpose.

>  }
>  
> -static void i2c_parport_detach(struct parport *port)
> +static int i2c_parport_probe(struct device *dev)
>  {
> -	struct i2c_par *adapter, *_n;
> +	char *name = dev_name(dev);

This adds the following warning at build time:

  CC [M]  drivers/i2c/busses/i2c-parport.o
drivers/i2c/busses/i2c-parport.c: In function ‘i2c_parport_probe’:
drivers/i2c/busses/i2c-parport.c:274:15: warning: initialization discards ‘const’ qualifier from pointer target type [enabled by default]
  char *name = dev_name(dev);

Very easy to fix, just declare "name" as const char *.

>  
> -	/* Walk the list */
> -	mutex_lock(&adapter_list_lock);
> -	list_for_each_entry_safe(adapter, _n, &adapter_list, node) {
> -		if (adapter->pdev->port == port) {
> -			if (adapter->ara) {
> -				parport_disable_irq(port);
> -				i2c_unregister_device(adapter->ara);
> -			}
> -			i2c_del_adapter(&adapter->adapter);
> -
> -			/* Un-init if needed (power off...) */
> -			if (adapter_parm[type].init.val)
> -				line_set(port, 0, &adapter_parm[type].init);
> -
> -			parport_release(adapter->pdev);
> -			parport_unregister_device(adapter->pdev);
> -			list_del(&adapter->node);
> -			kfree(adapter);
> -		}
> -	}
> -	mutex_unlock(&adapter_list_lock);
> +	if (is_parport(dev))
> +		return -ENODEV;
> +
> +	if (strlen(name) != 12 || strncmp(dev_name(dev), "i2c-parport", 11))

You already have the result of dev_name(dev) as "name" so don't call it
again.

This piece of code looks awkward, I am not aware of any subsystem
working that way. Look at the platform device driver subsystem for
example, or even i2c, the match is done on the full name, and several
devices can have the same name. The way to distinguish between them is
with their address or a device instance number, NOT by appending a
number at the end of the name string.

> +		return -ENODEV;
> +
> +	return 0;
>  }
>  
>  static struct parport_driver i2c_parport_driver = {
>  	.name	= "i2c-parport",
> -	.attach	= i2c_parport_attach,
> -	.detach	= i2c_parport_detach,
> +	.match_port	= i2c_parport_attach,
> +	.probe	= i2c_parport_probe,
>  };

The lack of detach function in the new model is suspicious, more on
that below.

>  
>  /* ----- Module loading, unloading and information ------------------------ */
> @@ -286,11 +302,34 @@ static int __init i2c_parport_init(void)
>  		return -ENODEV;
>  	}
>  
> -	return parport_register_driver(&i2c_parport_driver);
> +	return parport_register_drv(&i2c_parport_driver);

Can't you call parport_register_driver() for both models and decide
what to do based on which members of struct parport_driver are set?
This would be less confusing IMHO.

>  }
>  
>  static void __exit i2c_parport_exit(void)
>  {
> +	struct i2c_par *adapter, *_n;
> +
> +	/* Walk the list */
> +	mutex_lock(&adapter_list_lock);
> +	list_for_each_entry_safe(adapter, _n, &adapter_list, node) {
> +		if (adapter->ara) {
> +			parport_disable_irq(adapter->pdev->port);
> +			i2c_unregister_device(adapter->ara);
> +		}
> +		i2c_del_adapter(&adapter->adapter);
> +
> +		/* Un-init if needed (power off...) */
> +		if (adapter_parm[type].init.val)
> +			line_set(adapter->pdev->port, 0,
> +				 &adapter_parm[type].init);
> +
> +		parport_release(adapter->pdev);
> +		parport_unregister_device(adapter->pdev);
> +		list_del(&adapter->node);
> +		kfree(adapter);
> +	}
> +	mutex_unlock(&adapter_list_lock);

Moving all this code here seems inappropriate. What if a parallel port
is removed from the system while the i2c-parport driver is loaded? I
think this can happen with laptop docking stations or parallel ports on
PCI cards for example. Your new model should be able to deal with that,
so each driver still needs a detach or remove function which the core
can call on parallel port removal.

> +
>  	parport_unregister_driver(&i2c_parport_driver);
>  }
>  
> diff --git a/drivers/i2c/busses/i2c-parport.h b/drivers/i2c/busses/i2c-parport.h
> index 4e12945..00655f4 100644
> --- a/drivers/i2c/busses/i2c-parport.h
> +++ b/drivers/i2c/busses/i2c-parport.h
> @@ -104,3 +104,10 @@ MODULE_PARM_DESC(type,
>  	" 6 = Barco LPT->DVI (K5800236) adapter\n"
>  	" 7 = One For All JP1 parallel port adapter\n"
>  );
> +
> +static int parport[4] = {0, -1, -1, -1};
> +module_param_array(parport, int, NULL, 0);
> +MODULE_PARM_DESC(parport, "Atmost 4 instances are allowed.\n"
> +			  "Mention portnumbers in array.\n"
> +			  "If the port is not to be used mention -1.\n"
> +			  "Default is one instance connected to parport0\n");

This should go in i2c-parport.c, not i2c-parport.h. i2c-parport.h is
shared between the i2c-parport and i2c-parport-light drivers, and this
parameter is irrelevant for the latter.

Ideally the support for multiple instances and the conversion to the
new parport device model should also be added in separate patches.

-- 
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists