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

Naman Jain
2025-04-15T16:11:57Z

On 4/15/2025 9:38 PM, Michael Kelley wrote:
> 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>

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

Regards,
Naman