Re: [GIT PULL] FPGA Manager changes for 6.6-final

[Marco Pagani <marpagan@redhat.com>]

On 2023-10-18 13:50, Greg KH wrote:
> On Wed, Oct 18, 2023 at 11:39:01AM +0200, Marco Pagani wrote:
>>
>>
>> On 18/10/23 09:50, Greg KH wrote:
>>> On Wed, Oct 18, 2023 at 10:02:08AM +0800, Xu Yilun wrote:
>>>> On Tue, Oct 17, 2023 at 07:17:29PM +0200, Greg KH wrote:
>>>>> On Tue, Oct 17, 2023 at 11:00:22PM +0800, Xu Yilun wrote:
>>>>>> The following changes since commit 6465e260f48790807eef06b583b38ca9789b6072:
>>>>>>
>>>>>>   Linux 6.6-rc3 (2023-09-24 14:31:13 -0700)
>>>>>>
>>>>>> are available in the Git repository at:
>>>>>>
>>>>>>   git://git.kernel.org/pub/scm/linux/kernel/git/fpga/linux-fpga tags/fpga-for-6.6-final
>>>>>>
>>>>>> for you to fetch changes up to 6a935361500a21ef11a82814ee66fc58e59813f7:
>>>>>>
>>>>>>   fpga: Fix memory leak for fpga_region_test_class_find() (2023-10-12 12:59:29 +0800)
>>>>>>
>>>>>> ----------------------------------------------------------------
>>>>>> FPGA Manager changes for 6.6-final
>>>>>>
>>>>>> FPGA KUnit test:
>>>>>>
>>>>>> - Marco's change fixes null-ptr-deref when try_module_get()
>>>>>> - Jinjie's change fixes a memory leak issue
>>>>>>
>>>>>> Intel m10 bmc secure update:
>>>>>>
>>>>>> - Maintainer change from Russ Weight to Peter Colberg
>>>>>>
>>>>>> All patches have been reviewed on the mailing list, and have been in the
>>>>>> last linux-next releases (as part of our fixes branch)
>>>>>>
>>>>>> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
>>>>>>
>>>>>> ----------------------------------------------------------------
>>>>>> Jinjie Ruan (1):
>>>>>>       fpga: Fix memory leak for fpga_region_test_class_find()
>>>>>>
>>>>>> Marco Pagani (4):
>>>>>>       fpga: add helpers for the FPGA KUnit test suites.
>>>>>>       fpga: add a platform driver to the FPGA Manager test suite
>>>>>>       fpga: add a platform driver to the FPGA Bridge test suite
>>>>>>       fpga: add a platform driver to the FPGA Region test suite
>>>>>
>>>>> Why are all of these test suite patches here?  They are not relevant for
>>>>> 6.6-final as they do not resolve anything.
>>>>
>>>> Maybe the subjects indicate no bug fixing, but they fix null-ptr-deref
>>>> issues when modprobe fpga-mgr/bridge/region-test.
>>>
>>> That's not obvious, sorry.  So are the tests broken right now so that
>>> they don't work at all?
>>
>> They were broken only when compiled and run as modules.
> 
> Then forbid the ability to compile them as modules.

I was mistaken. To be more precise, the suites run when using the KUnit
default UML kernel but crashes on other archs unless MODULES=n.

> 
>>>> In fpga-mgr-test, the pdev->dev->driver is not assigned, so when
>>>>
>>>>   fpga_mgr_test_get()->try_module_get(dev->parent->driver->owner)
>>>
>>> That's a horrible line and should be fixed.  How do you know if a device
>>> has a parent, or if that parent has a driver?  You don't, that should be
>>> fixed instead.
>>>
>>> And module_get on a driver pointer is also never a good idea for other
>>> reasons, why is this happening at all?  It shouldn't be needed if the
>>> code is set up properly (i.e. the unloading of a driver will handle the
>>> shutdown and reference counting properly, no need to try to use module
>>> references at all.)
>>
>> You are right, but fixing the fpga core is outside the scope of my patches.
> 
> Which is why I'm not going to take these as you aren't fixing the root
> problem here :)
> 
>> My intent was to improve the test suite by adding fake drivers irrespective
>> of the fpga core quirks. I might try to send an RFC later to improve the
>> fpga core.
>>
>>>
>>>> NULL ptr is referenced.
>>>>
>>>> So do fpga-bridge/region-test.
>>>>
>>>> Patch #1 adds a common helper to generate a platform driver.
>>>
>>> Don't abuse platform devices/drivers like this, this is not a platform
>>> device or driver.  If you really want to do this, use a real driver and
>>> device, not a platform one please.
>>
>> Other test suites, like DRM suites, already use fake platform devices and
>> drivers. Moreover, many real FPGA IPs, like reconfiguration controllers and
>> bridges, are indeed modeled as platform devices. What is the benefit of
>> using a real driver and device?
> 
> Again, please do not abuse platform devices and drivers when they should
> not be used.  I can't catch all abuses, but when I do see them, I do
> object to them.

Could you please elaborate a little more on why using platform drivers
and devices for test cases is an abuse so I can improve the test suite?

> 
> And again, the root problem here isn't even a platform device issue,
> it's a "assuming the parent has a driver and we want to increment a
> module reference" issue.  That's not ok, nor even needed at all.

Thanks,
Marco