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

Michael Kelley
2025-04-15T16:08:17Z

From: Naman Jain <namjain@linux.microsoft.com> 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@linux.microsoft.com/
> * 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@linux.microsoft.com/
> 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@linux.microsoft.com/
> * 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@linux.microsoft.com/
> * 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@outlook.com>
Tested-by: Michael Kelley <mhklinux@outlook.com>