Re: [PATCH v4 0/2] uio_hv_generic: Fix ring buffer sysfs creation path

[Naman Jain]

On 4/15/2025 9:38 PM, Michael Kelley wrote:
> From: Naman Jain <namjain@xxxxxxxxxxxxxxxxxxx> Sent: Wednesday, April 9, 2025 11:09 PM
> > Hi,
> > This patch series aims to address the sysfs creation issue for the ring
> > buffer by reorganizing the code. Additionally, it updates the ring sysfs
> > size to accurately reflect the actual ring buffer size, rather than a
> > fixed static value.
> >
> > PFB change logs:
> >
> > Changes since v3:
> > https://lore.kernel.org/all/20250328052745.1417-1-namjain@xxxxxxxxxxxxxxxxxxx/
> > * Addressed Michael's comments regarding handling of return value of
> > sysfs_update_group in uio_hv_generic.
> >
> > Changes since v2:
> > https://lore.kernel.org/all/20250318061558.3294-1-namjain@xxxxxxxxxxxxxxxxxxx/
> > Addressed Greg's comments:
> > * Split the original patch into two.
> > * Updated the commit message to explain the problem scenario.
> > * Added comments for new APIs in the kerneldoc format.
> > * Highlighted potential race conditions and explained why sysfs should not be created in
> > the
> >    driver probe.
> >
> > * Made minor changes to how the sysfs_update_group return value is handled.
> >
> > Changes since v1:
> > https://lore.kernel.org/all/20250225052001.2225-1-namjain@xxxxxxxxxxxxxxxxxxx/
> > * Fixed race condition in setting channel->mmap_ring_buffer by
> >    introducing a new variable for visibility of sysfs (addressed Greg's
> >    comments)
> > * Used binary attribute fields instead of regular ones for initializing attribute_group.
> > * Make size of ring sysfs dynamic based on actual ring buffer's size.
> > * Preferred to keep mmap function in uio_hv_generic to give more control over ring's
> >    mmap functionality, since this is specific to uio_hv_generic driver.
> > * Remove spurious warning during sysfs creation in uio_hv_generic probe.
> > * Added comments in a couple of places.
> >
> > Changes since RFC patch:
> > https://lore.kernel.org/all/20250214064351.8994-1-namjain@xxxxxxxxxxxxxxxxxxx/
> > * Different approach to solve the problem is proposed (credits to
> >    Michael Kelley).
> > * Core logic for sysfs creation moved out of uio_hv_generic, to VMBus
> >    drivers where rest of the sysfs attributes for a VMBus channel
> >    are defined. (addressed Greg's comments)
> > * Used attribute groups instead of sysfs_create* functions, and bundled
> >    ring attribute with other attributes for the channel sysfs.
> >
> > Error logs:
> >
> > [   35.574120] ------------[ cut here ]------------
> > [   35.574122] WARNING: CPU: 0 PID: 10 at fs/sysfs/file.c:591 sysfs_create_bin_file+0x81/0x90
> > [   35.574168] Workqueue: hv_pri_chan vmbus_add_channel_work
> > [   35.574172] RIP: 0010:sysfs_create_bin_file+0x81/0x90
> > [   35.574197] Call Trace:
> > [   35.574199]  <TASK>
> > [   35.574200]  ? show_regs+0x69/0x80
> > [   35.574217]  ? __warn+0x8d/0x130
> > [   35.574220]  ? sysfs_create_bin_file+0x81/0x90
> > [   35.574222]  ? report_bug+0x182/0x190
> > [   35.574225]  ? handle_bug+0x5b/0x90
> > [   35.574244]  ? exc_invalid_op+0x19/0x70
> > [   35.574247]  ? asm_exc_invalid_op+0x1b/0x20
> > [   35.574252]  ? sysfs_create_bin_file+0x81/0x90
> > [   35.574255]  hv_uio_probe+0x1e7/0x410 [uio_hv_generic]
> > [   35.574271]  vmbus_probe+0x3b/0x90
> > [   35.574275]  really_probe+0xf4/0x3b0
> > [   35.574279]  __driver_probe_device+0x8a/0x170
> > [   35.574282]  driver_probe_device+0x23/0xc0
> > [   35.574285]  __device_attach_driver+0xb5/0x140
> > [   35.574288]  ? __pfx___device_attach_driver+0x10/0x10
> > [   35.574291]  bus_for_each_drv+0x86/0xe0
> > [   35.574294]  __device_attach+0xc1/0x200
> > [   35.574297]  device_initial_probe+0x13/0x20
> > [   35.574315]  bus_probe_device+0x99/0xa0
> > [   35.574318]  device_add+0x647/0x870
> > [   35.574320]  ? hrtimer_init+0x28/0x70
> > [   35.574323]  device_register+0x1b/0x30
> > [   35.574326]  vmbus_device_register+0x83/0x130
> > [   35.574328]  vmbus_add_channel_work+0x135/0x1a0
> > [   35.574331]  process_one_work+0x177/0x340
> > [   35.574348]  worker_thread+0x2b2/0x3c0
> > [   35.574350]  kthread+0xe3/0x1f0
> > [   35.574353]  ? __pfx_worker_thread+0x10/0x10
> > [   35.574356]  ? __pfx_kthread+0x10/0x10
> >
> > Regards,
> > Naman
> >
> > Naman Jain (2):
> >    uio_hv_generic: Fix sysfs creation path for ring buffer
> >    Drivers: hv: Make the sysfs node size for the ring buffer dynamic
> >
> >   drivers/hv/hyperv_vmbus.h    |   6 ++
> >   drivers/hv/vmbus_drv.c       | 119 ++++++++++++++++++++++++++++++++++-
> >   drivers/uio/uio_hv_generic.c |  39 +++++-------
> >   include/linux/hyperv.h       |   6 ++
> >   4 files changed, 147 insertions(+), 23 deletions(-)
>
> I've tested this series with linux-next20250411. Did the basics of binding the
> uio_hv_generic driver to the FCOPY device. The "ring" sysfs attribute shows up
> correctly, and with the expected size of 32 KiB. Did the same with a synthetic
> networking device, and the "ring" attribute has the expected size of 4 MiB. In both
> cases opened and mmap'ed the "ring" attribute, then read from the ring to
> verify accessibility.  Did not do any ring operations or create any subchannels.
>
> Did some test hackery to create the conditions where hv_create_ring_sysfs()
> could fail because the subdirectory under "channels" had not yet been created.
> hv_uio_probe() correctly ignores the error. When the subdirectory under
> "channels" is created later in vmbus_device_register(), the "ring" attribute
> appears with the correct size.
>
> Just saw Greg KH's comment about running checkpatch.pl.  I did not see any
> errors from checkpatch.pl.  Maybe that comment should have been directed
> to a different patch?
>
> For the series:
>
> Reviewed-by: Michael Kelley <mhklinux@xxxxxxxxxxx>
> Tested-by: Michael Kelley <mhklinux@xxxxxxxxxxx>

Thank you so much Michael for reviewing and testing this thoroughly.
Really appreciate it.

Regards,
Naman