netdev - Re: [PATCH net-next] ieee802154: use helper function to get rid of redundancy

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]

Date:	Sun, 1 Jun 2014 18:39:28 +0200
From:	Alexander Aring <alex.aring@...il.com>
To:	Jean Sacren <sakiwit@...il.com>
Cc:	Alexander Smirnov <alex.bluesman.smirnov@...il.com>,
	Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
	linux-zigbee-devel@...ts.sourceforge.net, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] ieee802154: use helper function to get rid of
 redundancy

On Sun, Jun 01, 2014 at 09:35:47AM -0600, Jean Sacren wrote:
> From: Alexander Aring <alex.aring@...il.com>
> Date: Sun, 01 Jun 2014 16:35:53 +0200
> >
> > Hi,
> > 
> > On Sun, Jun 01, 2014 at 08:23:17AM -0600, Jean Sacren wrote:
> > > From: Alexander Aring <alex.aring@...il.com>
> > > Date: Sun, 01 Jun 2014 09:26:57 +0200
> > > 
> > > Hi Alex,
> > > 
> > > Thank you very much for the feedback.
> > > 
> > > > the at86rf230 driver supports several at86rf2xx chips. You split the
> > > > at86rf212_set_channel which is at86rf212 specific in two function which
> > > > are named at86rf230_foo.
> > > 
> > > I didn't "split" at86rf212_set_channel() in two functions. I spliced
> > > those two sections of code and made at86rf212_set_channel() far
> > > succinct and easy to read.
> > > 
> > 
> > yes, but this driver supports more than one chip and it's easier to read
> > if we have one channel_set function for each chip type. Note you also
> > named the specific channel_set function to a another at86rf230_foo
> > function which is at86rf212 specific only. Sorry that will confuse
> > all the people who will ever read this code.
> > 
> > There is a at86rf230_ops and at86rf212_ops struct. The channel_set
> > function it's much easier to have only one callback for each struct,
> > otherwise you have 4 different channel_set functions and nobody knows
> > for which at86rf2xx type that function is for.
> 
> You mean something like the following will be less confusing?

Yes that's less but there are issues and I don't see any reason why we
should do that.

> 
> diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> index 4517b149ed07..06b494bacc44 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -602,20 +602,21 @@ at86rf212_set_channel(struct at86rf230_local *lp, int page, int channel)
>  {
>  	int rc;
>  
> -	if (channel == 0)
> -		rc = at86rf230_write_subreg(lp, SR_SUB_MODE, 0);
> -	else
> -		rc = at86rf230_write_subreg(lp, SR_SUB_MODE, 1);
> +	if (channel)
> +		channel = 1;
> +
> +	rc = at86rf230_write_subreg(lp, SR_SUB_MODE, channel);
>  	if (rc < 0)
>  		return rc;
>  

First:

This will break the at86rf212_set_channel function. At the end of this
function we need the channel parameter and you overwrite it here.
At the end of this function stands:

"return at86rf230_write_subreg(lp, SR_CHANNEL, channel);"

Second:

The variable channel should be a new variable named sub_mode and initialized
to 0, this fixes the first issue.

But again, I don't see any reasons why we should change that. It's the
same thing like before.

- Alex
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html