linux-kernel - Re: [PATCH v5 WIP 3/5] i2c-parport: define ports to connect

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1432027695.24979.55.camel@chaos.site>
Date:	Tue, 19 May 2015 11:28:15 +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 v5 WIP 3/5] i2c-parport: define ports to connect

Le Tuesday 19 May 2015 à 14:14 +0530, Sudip Mukherjee a écrit :
> On Tue, May 19, 2015 at 09:50:47AM +0200, Jean Delvare wrote:
> > Hi Sudip,
> > 
> > Le Wednesday 06 May 2015 à 15:46 +0530, Sudip Mukherjee a écrit :
> > > as of now i2c-parport was connecting to all the available parallel
> > > ports. Lets limit that to maximum of 4 instances and at the same time
> > > define which instance connects to which parallel port
> > 
> > A leading capital and a trailing dot would look better.
> sure.
> > 
> > > Signed-off-by: Sudip Mukherjee <sudip@...torindia.org>
> > > ---
> <snip>
> > > +	for (i = 0; i < MAX_DEVICE; i++) {
> > > +		if (parport[i] == -1)
> > > +			continue;
> > > +		if (port->number == parport[i])
> > > +			break;
> > > +	}
> > > +	if (i == MAX_DEVICE) {
> > > +		pr_err("port mentioned not found\n");
> > 
> > This error message needs to be improved. Someone seeing this in his/her
> > logs would have no idea where it comes from and what it means exactly.
> > You want to add "i2c-parport: " at the beginning, and say which port
> > number was specified.
> oops. sorry about it. I saw all the other messages are having
> "i2c-parport: " mentioned. I will modify it. And maybe later I will
> send you another patch to use pr_fmt.

Using pr_fmt would be great indeed, but later as a separate patch.

> Or here it may be better if I
> mention:
> pr_err("i2c-parport: You have chosen not to use parport%d.\n",
> 	port->number);

Ah, now I realize that the original error message was misleading. In
fact you should not print any error message here (maybe only a debug
message if you really want.) There's nothing wrong with not binding to a
specific port, actually the whole point of this patch is to allow the
user to do exactly that.

> > 
> > > +		return;
> > > +	}
> > >  
> > >  	adapter = kzalloc(sizeof(struct i2c_par), GFP_KERNEL);
> > >  	if (adapter == NULL) {
> > > @@ -298,5 +313,11 @@ MODULE_AUTHOR("Jean Delvare <jdelvare@...e.de>");
> > >  MODULE_DESCRIPTION("I2C bus over parallel port");
> > >  MODULE_LICENSE("GPL");
> > >  
> > > +module_param_array(parport, int, NULL, 0);
> > > +MODULE_PARM_DESC(parport, "Atmost 4 instances are allowed.\n"
> > 
> > You should first say what the parameter does, before going into the
> > details. Please use __stringify(MAX_DEVICE) instead of hard-coding 4, so
> > that it doesn't need to be updated if MAX_DEVICE ever changes.
> then what about:
> MODULE_PARM_DESC(parport, "Mention number of i2c-parport instances you
> 		 want.\n Atmost " __stringify(MAX_DEVICE) " instances are
> 		 allowed.\n Mention port numbers you want to use.\n"
> 		 " If the port is not to be used mention -1.\n"
> 		 " Default is one instance connected to parport0.\n");

I suggest formatting this in such a way that "\n"s are at the end of
lines (as your patch had.) This makes this much more readable and
unexpected embedded line breaks and spaces as you have here (the output
of modinfo would look ugly.)

Also the initial description is still misleading, and there are too many
occurrences of "mention" to my taste. I'd suggest the following:

MODULE_PARM_DESC(parport,
	"List of parallel ports to bind to, by index.\n"
	" Atmost " __stringify(MAX_DEVICE) " devices are supported.\n"
	" Default is one device connected to parport0.\n"
);

I have removed the mention of -1 as this is an internal implementation
detail and I can't think of any reason why the user would ever want to
pass it.


> > 
> <snip>
> > 
> > I tested this patch and it works fine.
> > 
> > Tested-by: Jean Delvare <jdelvare@...e.de>
> 
> do i add your Tested-by: to the main patch and this patch?

Yes.

-- 
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/