netdev - RE: [PATCH lora-next 01/10] net: lora: sx1301: add register, bit-fields, and helpers for regmap

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: <BY1PR02MB111463B66B79C5008905E011E7250@BY1PR02MB1114.namprd02.prod.outlook.com>
Date:   Thu, 9 Aug 2018 13:23:25 +0000
From:   Ben Whitten <Ben.Whitten@...rdtech.com>
To:     Andreas Färber <afaerber@...e.de>,
        Ben Whitten <ben.whitten@...il.com>
CC:     "starnight@...cu.edu.tw" <starnight@...cu.edu.tw>,
        "hasnain.virk@....com" <hasnain.virk@....com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Xue Liu <liuxuenetmail@...il.com>,
        Sebastian Heß <shess@...sware.de>
Subject: RE: [PATCH lora-next 01/10] net: lora: sx1301: add register,
 bit-fields, and helpers for regmap

> To: Ben Whitten <Ben.Whitten@...rdtech.com>; Ben
> Whitten <ben.whitten@...il.com>
> Cc: starnight@...cu.edu.tw; hasnain.virk@....com;
> netdev@...r.kernel.org; Xue Liu
> <liuxuenetmail@...il.com>; Sebastian Heß
> <shess@...sware.de>
> Subject: Re: [PATCH lora-next 01/10] net: lora: sx1301: add
> register, bit-fields, and helpers for regmap
> 
> + Xue Liu + Sebastian
> 
> Am 08.08.2018 um 17:52 schrieb Ben Whitten:
> >> Subject: Re: [PATCH lora-next 01/10] net: lora: sx1301:
> add
> >> register, bit-fields, and helpers for regmap
> >>
> >> Am 08.08.2018 um 14:32 schrieb Ben Whitten:
> >>>>>  drivers/net/lora/Kconfig  |   1 +
> >>>>>  drivers/net/lora/sx1301.c | 282
> >>>>
> +++++++++++++++++++++++++++++++++++++++++++-
> >> --
> >>>>>  drivers/net/lora/sx1301.h | 169
> >>>> +++++++++++++++++++++++++++
> >>>>>  3 files changed, 439 insertions(+), 13 deletions(-)
> >>>>>  create mode 100644 drivers/net/lora/sx1301.h
> >>>>
> >>>> My main concern about this patch is its sheer size.
> >> Normally
> >>>> for
> >>>> #defines the rule is not to add unused ones. Here I
> see
> >> for
> >>>> example FSK
> >>>> RSSI fields that we're surely not using yet. Any chance
> to
> >>>> strip this
> >>>> down some more?
> >>>
> >>> Sure, I'll strip the reg_fields down to those only
> required
> >> for
> >>> loading firmware and setting clocks that will be used in
> the
> >>> immediate term. This does clutter the driver a bit
> >>> unnecessarily at the moment.
> >>> I would like to keep the full register dump in the header
> >> file
> >>> though as its self-contained and gives a full picture of
> the
> >> chip.
> >>
> >> Could you do that in more steps though? I'm thinking,
> >> convert only the
> >> registers in use to regmap (that'll make it easier to
> review),
> >> then add
> >> more registers, then convert to regmap fields?
> >
> > I split the conversion function by function but we can
> probably go
> > further if you think it helpful.
> 
> That split feels wrong...
> 
> > Is it more the addition of the replacement register defines
> that you
> > would like to correlate with what's being removed, so you
> can see
> > in one patch that this register has the same page and the
> same
> > address in the new system?
> 
> I am looking for patches that are trivial to review. One
> aspect only.
> So I would much rather get a large patch with a consistent
> series of
> 
> -my_read
> +your_read
> 
> -my_write
> +your_write
> 
> that's quick to review than lots of different refactorings
> mixed into
> one patch, grouped by their file location.
> 
> So I am suggesting that if, for example, you want to pass fw
> to helpers,
> which looks like a great idea, that should be a small patch of
> its own,
> at the very beginning of your series. (git rebase -i is your
> friend.)
> 
> Converting to regmap_read/write I imagine to be a trivial
> search-and-replace type refactoring, leaving my |= and &=
> operations in
> place, to minimize the diff and avoid it mis-detecting the
> patch context.
> Without caching I'd expect regmap to work up to here - if it
> doesn't,
> then we can't apply it to my tree and need to prioritize other
> cleanups
> while we review/debug further.
> 
> Once all registers use regmap successfully, we can optimize
> that code by
> introducing regmap fields. This could be split by location, if
> desired.
> 
> Finally in the end you can introduce more registers and
> fields for
> future use.
> 
> Does that make sense now?

Yep no problem, hopefully my V2 is suitable. I stopped short
of including all registers and doing any regmap_field work
in this series so that we can progress.
I have the sx125x split and conversion to regmap_bus for
the concentrator ready to go in the wings.

> > The problem I face is that these conversions are almost
> blind as
> > when I run on my hardware I either oops with the
> sx1301_read
> > or the cal firmware doesn't verify so I can't finish probe. I
> only
> > get a full sx1301 module probe pass on physical hardware
> when
> > I get right to the end of the series where it's all replaced
> with
> > regmap.
> 
> If patches don't build or don't work then I can't apply them.
> Otherwise
> the 0-day bots will spam us with error reports, as you've
> seen before.

Understood, each patch in v2 has been tested and I was able
to probe and remove modules.

> BTW we'll need this regmap conversion for the picoGW_hal,
> so once we
> have a working SPI regmap driver, we'll need to split out the
> SPI bits,
> similar to sx125x.

I am unfamiliar with the picoGW_hal, do they expose the sx1301
as a device on a regmap_bus then?

Regards,
Ben Whitten