[GIT PULL] FPGA Manager changes for 6.6-final

[GIT PULL] FPGA Manager changes for 6.6-final

[Xu Yilun]

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

Russ Weight (1):
      fpga: m10bmc-sec: Change contact for secure update driver

 .../testing/sysfs-driver-intel-m10-bmc-sec-update  | 14 +++++------
 MAINTAINERS                                        |  2 +-
 drivers/fpga/tests/fpga-bridge-test.c              | 18 +++++++++++++-
 drivers/fpga/tests/fpga-mgr-test.c                 | 18 +++++++++++++-
 drivers/fpga/tests/fpga-region-test.c              | 28 +++++++++++++++++----
 drivers/fpga/tests/fpga-test-helpers.h             | 29 ++++++++++++++++++++++
 6 files changed, 94 insertions(+), 15 deletions(-)
 create mode 100644 drivers/fpga/tests/fpga-test-helpers.h

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

[Greg KH]

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.

I can see the memory leak being relevant, and also:

> Russ Weight (1):
>       fpga: m10bmc-sec: Change contact for secure update driver

That one, but why the testing code?  What bug/regression are they
fixing?

thanks,

greg k-h

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

[Xu Yilun]

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.

In fpga-mgr-test, the pdev->dev->driver is not assigned, so when

  fpga_mgr_test_get()->try_module_get(dev->parent->driver->owner)

NULL ptr is referenced.

So do fpga-bridge/region-test.

Patch #1 adds a common helper to generate a platform driver.
Patch #2/3/4 fix the issues by matching the driver to pdev.

See:
Closes: https://lore.kernel.org/linux-fpga/4d51e87f-830a-adae-d6f7-6aed9433fdc6@huawei.com/
Closes: https://lore.kernel.org/linux-fpga/f2b30203-1a67-4533-eddc-b380044e2e68@huawei.com/
Closes: https://lore.kernel.org/linux-fpga/d557b4ee-4b3a-8747-bdda-0ed480212a63@huawei.com/

Thanks,
Yilun

> 
> I can see the memory leak being relevant, and also:
> 
> > Russ Weight (1):
> >       fpga: m10bmc-sec: Change contact for secure update driver
> 
> That one, but why the testing code?  What bug/regression are they
> fixing?
> 
> thanks,
> 
> greg k-h

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

[Greg KH]

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?

> 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.)

> 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.

> Patch #2/3/4 fix the issues by matching the driver to pdev.
> 
> See:
> Closes: https://lore.kernel.org/linux-fpga/4d51e87f-830a-adae-d6f7-6aed9433fdc6@huawei.com/
> Closes: https://lore.kernel.org/linux-fpga/f2b30203-1a67-4533-eddc-b380044e2e68@huawei.com/
> Closes: https://lore.kernel.org/linux-fpga/d557b4ee-4b3a-8747-bdda-0ed480212a63@huawei.com/

Want to just disable all of these from the build entirely for now and
then fix them up properly for 6.7?

thanks,

greg k-h

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

[Marco Pagani]

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.

> 
>> 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.
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?

> 
>> Patch #2/3/4 fix the issues by matching the driver to pdev.
>>
>> See:
>> Closes: https://lore.kernel.org/linux-fpga/4d51e87f-830a-adae-d6f7-6aed9433fdc6@huawei.com/
>> Closes: https://lore.kernel.org/linux-fpga/f2b30203-1a67-4533-eddc-b380044e2e68@huawei.com/
>> Closes: https://lore.kernel.org/linux-fpga/d557b4ee-4b3a-8747-bdda-0ed480212a63@huawei.com/
> 
> Want to just disable all of these from the build entirely for now and
> then fix them up properly for 6.7?
> 
> thanks,
> 
> greg k-h
> 

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

[Greg KH]

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.

> >> 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.

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,

greg k-h

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

[Marco Pagani]

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

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

[Greg KH]

On Wed, Oct 18, 2023 at 05:40:08PM +0200, Marco Pagani wrote:
> 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:
> >>>> 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?

Because they just are not platform devices.

A platform driver is one that has hardware resources like DT or ACPI or
other real resources that talk to hardware and are not on a discoverable
bus, they are NOT fake devices with no actual hardware resources.

If you need a "fake" device, make a fake device, that's what virtual
devices are for there are other solutions as well depending on how you
want to use it.

thanks,

greg k-h

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

[Jinjie Ruan]

On 2023/10/18 19: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.

There is also a null-ptr-deref for fpga-mgr/bridge/region-test when
CONFIG_FPGA_KUNIT_TESTS=y

[  115.526321] ok 76 rtc_lib_test_cases
[  115.528830]     KTAP version 1
[  115.529612]     # Subtest: fpga_mgr
[  115.530500]     # module: fpga_mgr_test
[  115.530507]     1..4
[  115.533848] general protection fault, probably for non-canonical
address 0xdffffc0000000002: 0000 [#1] PREEMPT SMP KASAN
[  115.536884] KASAN: null-ptr-deref in range
[0x0000000000000010-0x0000000000000017]
[  115.538778] CPU: 2 PID: 2171 Comm: kunit_try_catch Tainted: G    B
W        N 6.6.0-rc6+ #125
[  115.540918] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.13.0-1ubuntu1.1 04/01/2014
[  115.543163] RIP: 0010:__fpga_mgr_get+0x63/0xb0
[  115.544290] Code: 48 8d 7b 68 48 89 fa 48 c1 ea 03 80 3c 02 00 75 52
48 b8 00 00 00 00 00 fc ff df 48 8b 5b 68 48 8d 7b 10 48 89 fa 48 c1 ea
03 <807b 10 e8 7e b1 97 fd 84 c0 74 08 4c 89 e0
[  115.548872] RSP: 0000:ffff88810909fe18 EFLAGS: 00010202
[  115.550179] RAX: dffffc0000000000 RBX: 0000000000000000 RCX:
ffffffff8402d6ac
[  115.551944] RDX: 0000000000000002 RSI: 0000000000000004 RDI:
0000000000000010
[  115.553705] RBP: ffff8881078ba808 R08: ffffed1021213f96 R09:
ffffed10203c0acb
[  115.555465] R10: ffffed10203c0aca R11: ffff888101e05653 R12:
ffff8881078ba800
[  115.557222] R13: ffff8881064bc958 R14: ffff888100b57b20 R15:
ffff888109b45440
[  115.559000] FS:  0000000000000000(0000) GS:ffff888119f00000(0000)
knlGS:0000000000000000
[  115.560989] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  115.562427] CR2: ffff888119750000 CR3: 0000000005285001 CR4:
0000000000170ee0
[  115.564207] DR0: ffffffff8fe17ce0 DR1: ffffffff8fe17ce1 DR2:
ffffffff8fe17ce3
[  115.565973] DR3: ffffffff8fe17ce5 DR6: 00000000ffff0ff0 DR7:
0000000000000600
[  115.567757] Call Trace:
[  115.568372]  <TASK>
[  115.568926]  ? __die_body+0x1b/0x60
[  115.569812]  ? die_addr+0x43/0x70
[  115.570651]  ? exc_general_protection+0x121/0x210
[  115.571849]  ? asm_exc_general_protection+0x22/0x30
[  115.573069]  ? kobject_put+0x5c/0x310
[  115.573999]  ? __fpga_mgr_get+0x63/0xb0
[  115.574993]  fpga_mgr_test_get+0xb6/0x1b0
[  115.576008]  ? platform_device_register_simple.constprop.0+0xc0/0xc0
[  115.577580]  ? fpga_mgr_test_lock+0x200/0x200
[  115.578713]  ? kunit_try_run_case+0xdd/0x250
[  115.579794]  ? kunit_suite_has_succeeded+0x50/0x50
[  115.580990]  kunit_generic_run_threadfn_adapter+0x4a/0x90
[  115.582345]  ? kunit_try_catch_throw+0x80/0x80
[  115.583460]  kthread+0x2b6/0x380
[  115.584289]  ? kthread_complete_and_exit+0x20/0x20
[  115.585485]  ret_from_fork+0x2d/0x70
[  115.586407]  ? kthread_complete_and_exit+0x20/0x20
[  115.587607]  ret_from_fork_asm+0x11/0x20
[  115.588595]  </TASK>
[  115.589165] Modules linked in:
[  115.589954] Dumping ftrace buffer:
[  115.590854]    (ftrace buffer empty)
[  115.591804] ---[ end trace 0000000000000000 ]---
[  115.592999] RIP: 0010:__fpga_mgr_get+0x63/0xb0
[  115.594152] Code: 48 8d 7b 68 48 89 fa 48 c1 ea 03 80 3c 02 00 75 52
48 b8 00 00 00 00 00 fc ff df 48 8b 5b 68 48 8d 7b 10 48 89 fa 48 c1 ea
03 <807b 10 e8 7e b1 97 fd 84 c0 74 08 4c 89 e0
[  115.598801] RSP: 0000:ffff88810909fe18 EFLAGS: 00010202
[  115.600119] RAX: dffffc0000000000 RBX: 0000000000000000 RCX:
ffffffff8402d6ac
[  115.601900] RDX: 0000000000000002 RSI: 0000000000000004 RDI:
0000000000000010
[  115.603690] RBP: ffff8881078ba808 R08: ffffed1021213f96 R09:
ffffed10203c0acb
[  115.605460] R10: ffffed10203c0aca R11: ffff888101e05653 R12:
ffff8881078ba800
[  115.607253] R13: ffff8881064bc958 R14: ffff888100b57b20 R15:
ffff888109b45440
[  115.609047] FS:  0000000000000000(0000) GS:ffff888119f00000(0000)
knlGS:0000000000000000
[  115.611063] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  115.612500] CR2: ffff888119750000 CR3: 0000000005285001 CR4:
0000000000170ee0
[  115.614301] DR0: ffffffff8fe17ce0 DR1: ffffffff8fe17ce1 DR2:
ffffffff8fe17ce3
[  115.616088] DR3: ffffffff8fe17ce5 DR6: 00000000ffff0ff0 DR7:
0000000000000600
[  115.617890] Kernel panic - not syncing: Fatal exception
[  115.619425] Dumping ftrace buffer:
[  115.620271]    (ftrace buffer empty)
[  115.621151] Kernel Offset: disabled
[  115.622007] Rebooting in 1 seconds..


> 
>>>> 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.
> 
> 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,
> 
> greg k-h

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

[Xu Yilun]

On Wed, Oct 18, 2023 at 09:50:37AM +0200, 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?
> 
> > 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.)
> 
> > 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.
> 
> > Patch #2/3/4 fix the issues by matching the driver to pdev.
> > 
> > See:
> > Closes: https://lore.kernel.org/linux-fpga/4d51e87f-830a-adae-d6f7-6aed9433fdc6@huawei.com/
> > Closes: https://lore.kernel.org/linux-fpga/f2b30203-1a67-4533-eddc-b380044e2e68@huawei.com/
> > Closes: https://lore.kernel.org/linux-fpga/d557b4ee-4b3a-8747-bdda-0ed480212a63@huawei.com/
> 
> Want to just disable all of these from the build entirely for now and
> then fix them up properly for 6.7?

I'm OK this way.

Macro, could you send a patch to Greg for diabling them? And let's do
proper fix up in FPGA core for 6.7.

Thanks,
Yilun

> 
> thanks,
> 
> greg k-h