linux-kernel - Re: [PATCH 1/8] staging: sm250fb: remove USE_HW

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: <ec791e981505d3c8e1d14e6d68eb616ffa4aea71.camel@aol.com>
Date: Fri, 18 Apr 2025 12:42:05 +0100
From: Ruben Wauters <rubenru09@....com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Sudip Mukherjee <sudipm.mukherjee@...il.com>, Teddy Wang
	 <teddy.wang@...iconmotion.com>, Sudip Mukherjee
	 <sudip.mukherjee@...ethink.co.uk>, linux-fbdev@...r.kernel.org, 
	linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/8] staging: sm250fb: remove USE_HW_I2C check

On Fri, 2025-04-18 at 12:33 +0200, Greg Kroah-Hartman wrote:
> On Thu, Apr 17, 2025 at 08:02:49PM +0100, Ruben Wauters wrote:
> > Removes the USE_HW_I2C check and function defines in
> > ddk750_sii164.c.
> > 
> > The software equivalents were never used due to
> > USE_HW_I2C being defined just before the ifdef, meaning
> > the hardware versions were always used.
> > 
> > The define names were also triggering checkpatch.pl's
> > camel case check.
> > 
> > Signed-off-by: Ruben Wauters <rubenru09@....com>
> > 
> > ---
> > 
> > I am somewhat unsure whether this is the way to go or
> > the correct way would be to add an option/opportunity for
> > the software version to be used. Currently the hardware
> > version is always used, but I am unsure if there ever even
> > would be a case where you would want to use the software
> > version over the hardware version.
> 
> Then the code can be added back, not an issue.
> 
> But you forgot this same check in
> drivers/staging/sm750fb/ddk750_hwi2c.c, right?

This check can indeed be removed I suppose, might be worth also
removing the USE_DVICHIP checks also, especially when defined

There's also a USE_HW_I2C check in ddk750.h, which defines which header
to use, however I'm somewhat unsure exactly what the best way to go
about addressing that is, since it's not defined before including it
does make me wonder whether the HW files are even used at all.

Something about this seems entirely wrong to me, again I don't have the
hardware to test, but why would SW files be used when the HW files work
fine? Is it intended that the HW files are used instead? it's a bit
inconsistent, especially since in ddk750_sii164.c the HW ones are
explicitly used over the SW ones
Why is the SW files preferred in sm750_hw.c then?
I believe this is something of an oversight and the files should
probably use the HW ones instead of the SW ones.

I am curious to know your thoughts on this (and anyone else's)

> Also, what about removing the sm750_sw_i2c_write_reg() and other
> functions that are now never referenced?  Can you add that to this
> patch
> series?  A single series that just removes the use of USE_HW_I2C and
> the
> now unneeded functions would be best, as it's not really a "coding
> style" fix, but rather a code cleanup thing, right?

I will create a separate patch series for removing unneeded code, since
it does look like a lot more code removal might be needed than I
originally thought.

> thanks,
> 
> greg k-h