[PATCH] firewire: ohci: make reg_(read|write) unsigned

[PATCH] firewire: ohci: make reg_(read|write) unsigned

[Jordy Zomer]

The reg_(read|write) functions used to
take a signed integer as an offset parameter.
The callers of this function only pass an unsigned integer to it.
Therefore to make it obviously safe, let's just make this an unsgined
integer as this is used in pointer arithmetics.

Signed-off-by: Jordy Zomer <jordy@pwning.systems>
---
 drivers/firewire/ohci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index 17c9d825188b..0119aa9cbc7c 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -524,12 +524,12 @@ static void log_ar_at_event(struct fw_ohci *ohci,
 	}
 }
 
-static inline void reg_write(const struct fw_ohci *ohci, int offset, u32 data)
+static inline void reg_write(const struct fw_ohci *ohci, u32 offset, u32 data)
 {
 	writel(data, ohci->registers + offset);
 }
 
-static inline u32 reg_read(const struct fw_ohci *ohci, int offset)
+static inline u32 reg_read(const struct fw_ohci *ohci, u32 offset)
 {
 	return readl(ohci->registers + offset);
 }
-- 
2.27.0

Re: [PATCH] firewire: ohci: make reg_(read|write) unsigned

[Greg Kroah-Hartman]

On Sat, Jul 31, 2021 at 12:41:12PM +0200, Jordy Zomer wrote:
> The reg_(read|write) functions used to
> take a signed integer as an offset parameter.
> The callers of this function only pass an unsigned integer to it.
> Therefore to make it obviously safe, let's just make this an unsgined
> integer as this is used in pointer arithmetics.
> 
> Signed-off-by: Jordy Zomer <jordy@pwning.systems>
> ---
>  drivers/firewire/ohci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Same thing should probably also be done in
drivers/firewire/init_ohci1394_dma.c for the same inline functions,
right?

Anyway, nice cleanup:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH] firewire: ohci: make reg_(read|write) unsigned

[Greg Kroah-Hartman]

On Sun, Aug 01, 2021 at 08:24:04AM +0200, Greg Kroah-Hartman wrote:
> On Sat, Jul 31, 2021 at 12:41:12PM +0200, Jordy Zomer wrote:
> > The reg_(read|write) functions used to
> > take a signed integer as an offset parameter.
> > The callers of this function only pass an unsigned integer to it.
> > Therefore to make it obviously safe, let's just make this an unsgined
> > integer as this is used in pointer arithmetics.
> > 
> > Signed-off-by: Jordy Zomer <jordy@pwning.systems>
> > ---
> >  drivers/firewire/ohci.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Same thing should probably also be done in
> drivers/firewire/init_ohci1394_dma.c for the same inline functions,
> right?

And sound/firewire/isight.c also could use this.  Seems like there was
some copy/paste in firewire drivers :)

thanks,

greg k-h

Re: [PATCH] firewire: ohci: make reg_(read|write) unsigned

[Stefan Richter]

On Aug 01 Greg Kroah-Hartman wrote:
> On Sun, Aug 01, 2021 at 08:24:04AM +0200, Greg Kroah-Hartman wrote:
> > On Sat, Jul 31, 2021 at 12:41:12PM +0200, Jordy Zomer wrote:  
> > > The reg_(read|write) functions used to
> > > take a signed integer as an offset parameter.
> > > The callers of this function only pass an unsigned integer to it.
> > > Therefore to make it obviously safe, let's just make this an unsgined
> > > integer as this is used in pointer arithmetics.
> > > 
> > > Signed-off-by: Jordy Zomer <jordy@pwning.systems>
> > > ---
> > >  drivers/firewire/ohci.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)  
> > 
> > Same thing should probably also be done in
> > drivers/firewire/init_ohci1394_dma.c for the same inline functions,
> > right?  

Yes, register offsets are always non-negative; the lowest register address
is used as the base address.  All of the offset values are taken from
macros which are define in ohci.h.

> And sound/firewire/isight.c also could use this.  Seems like there was
> some copy/paste in firewire drivers :)

These offsets are non-negative too; they defined as macros at the top of
isight.c.  However, here we aren't in the 32 bit (?) PCI register space but
in the 48 bit FireWire node address space, which is why the functions which
are wrapped up by reg_read/write() --- snd_fw_transaction() and
fw_run_transaction() --- use u64 or unsigned long long for @offset.

Long story short, isight.c::reg_read/write() could use u32 or unsigned int
or u64 or unsigned long long for @offset; it's going to be added to an u64
so maybe that's what we should use right away?
-- 
Stefan Richter
-======--=-= =--- ----=
http://arcgraph.de/sr/