linux-kernel - Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20171205103008.h7evql7onlaygczi@mwanda>
Date:   Tue, 5 Dec 2017 15:16:29 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Marcus Wolf <marcus.wolf@...rthome-wolf.de>
Cc:     Simon Sandström <simon@...anor.nu>,
        devel@...verdev.osuosl.org, gregkh@...uxfoundation.org,
        linux@...f-Entwicklungen.de, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in
 rf69_enum.h

On Mon, Dec 04, 2017 at 09:59:02PM +0200, Marcus Wolf wrote:
> Keep in mind, that if you split the functions, in the interface
> implementation you also need more code:
> 
> SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
> 
> will have to be converted in something like
> 
> if (rx_cfg->enable_sync)
> 	SET_CHECKED(rf69_set_sync_enbable(dev->spi);
> else
> 	SET_CHECKED(rf69_set_sync_disable(dev->spi);
> 

Here's what the code looks like right now:

   198          /* packet config */
   199          /* enable */
   200          SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
   201          if (rx_cfg->enable_sync == optionOn)
   202          {
   203                  SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, afterSyncInterrupt));
   204          }
   205          else
   206          {
   207                  SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, always));
   208          }

That's for the rx_cfg.  We have related but different code for the
tx_cfg.  It's strange to me that we can enable sync for rx and not for
tx...  How does that work when the setting ends up getting stored in the
same register?

The new code would look like this:

	if (rx_cfg->enable_sync) {
		ret = rf69_enable_sync(spi);
		if (ret)
			return ret;
		ret = rf69_set_fifo_fill_condition(dev->spi, afterSyncInterrupt);
		if (ret)
			return ret;
	} else {
		ret = rf69_disable_sync(dev->spi);
		if (ret)
			return ret;
		ret = rf69_set_fifo_fill_condition(dev->spi, always);
		if (ret)
			return ret;
	}

It's not the greatest, but it's not the worst...  The configuration for
->enable_sync is a bit spread out and it might be nice to move it all to
one function?

I liked Simon's naming scheme and I thought it was clear what the
rf69_set_sync(spi, false) function would do.

Simon, it seems like Marcus and I both are Ok with your style choices.
Do whatever seems best when you implement the code.  If it's awkward to
break up the functions then don't.

regards,
dan carpenter