uio_hv_generic: Fix ring buffer sysfs creation path
v4 cover uio_hv_generic: Fix ring buffer sysfs creation path
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/[email protected]/
* 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/[email protected]/
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/[email protected]/
* 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/[email protected]/
* 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(-)
base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
Signature · 1 line
2.43.0
PATCH v4 1/2 uio_hv_generic: Fix sysfs creation path for ring buffer
On regular bootup, devices get registered to VMBus first, so when uio_hv_generic driver for a particular device type is probed, the device is already initialized and added, so sysfs creation in uio_hv_generic probe works fine. However, when device is removed and brought back, the channel rescinds and device again gets registered to VMBus. However this time, the uio_hv_generic driver is already registered to probe for that device and in this case sysfs creation is tried before the device's kobject gets initialized completely.
Fix this by moving the core logic of sysfs creation for ring buffer, from uio_hv_generic to HyperV's VMBus driver, where rest of the sysfs attributes for the channels are defined. While doing that, make use of attribute groups and macros, instead of creating sysfs directly, to ensure better error handling and code flow.
Problem path:vmbus_process_offer (new offer comes for the VMBus device)
vmbus_add_channel_work
vmbus_device_register
|-> device_register
| |...
| |-> hv_uio_probe
| |...
| |-> sysfs_create_bin_file (leads to a warning as
| primary channel's kobject, which is used to
| create sysfs is not yet initialized)
|-> kset_create_and_add
|-> vmbus_add_channel_kobj (initialization of primary channel's
kobject happens later)Above code flow is sequential and the warning is always reproducible in this path.
--- drivers/hv/hyperv_vmbus.h | 6 ++ drivers/hv/vmbus_drv.c | 110 ++++++++++++++++++++++++++++++++++- drivers/uio/uio_hv_generic.c | 39 ++++++------- include/linux/hyperv.h | 6 ++ 4 files changed, 138 insertions(+), 23 deletions(-)
drivers/hv/hyperv_vmbus.h +6 -0
@@ -477,4 +477,10 @@ static inline int hv_debug_add_dev_dir(struct hv_device *dev)#endif /* CONFIG_HYPERV_TESTING */ +/* Create and remove sysfs entry for memory mapped ring buffers for a channel */ +int hv_create_ring_sysfs (struct vmbus_channel *channel, +int (*hv_mmap_ring_buffer)(struct vmbus_channel *channel, +struct vm_area_struct *vma)); +int hv_remove_ring_sysfs (struct vmbus_channel *channel); +#endif /* _HYPERV_VMBUS_H */
drivers/hv/vmbus_drv.c +109 -1
136 lines not on this page · raw message
drivers/uio/uio_hv_generic.c +17 -22
@@ -131,15 +131,12 @@ static void hv_uio_rescind(struct vmbus_channel *channel)vmbus_device_unregister (channel->device_obj); } -/* Sysfs API to allow mmap of the ring buffers +/* Function used for mmap of ring buffer sysfs interface. * The ring buffer is allocated as contiguous memory by vmbus_open */ -static int hv_uio_ring_mmap (struct file *filp,struct kobject *kobj, -const struct bin_attribute *attr, -struct vm_area_struct *vma) +static int +hv_uio_ring_mmap (struct vmbus_channel *channel,struct vm_area_struct *vma) { -struct vmbus_channel *channel - = container_of(kobj, struct vmbus_channel, kobj);void *ring_buffer =page_address (channel->ringbuffer_page);if (channel->state !=CHANNEL_OPENED_STATE )
@@ -149,15 +146,6 @@ static int hv_uio_ring_mmap(struct file *filp, struct kobject *kobj, channel->ringbuffer_pagecount << PAGE_SHIFT); } -static const struct bin_attribute ring_buffer_bin_attr = { - .attr = { - .name ="ring" , - .mode =0600 , - }, - .size =2 *SZ_2M , - .mmap = hv_uio_ring_mmap, -}; -/* Callback from VMBUS subsystem when new channel created. */ static void hv_uio_new_channel (struct vmbus_channel *new_sc)
@@ -178,8 +166,7 @@ hv_uio_new_channel(struct vmbus_channel *new_sc)/* Disable interrupts on sub channel */ new_sc->inbound.ring_buffer->interrupt_mask =1 ;set_channel_read_mode (new_sc,HV_CALL_ISR ); - - ret =sysfs_create_bin_file (&new_sc->kobj, &ring_buffer_bin_attr); + ret =hv_create_ring_sysfs (new_sc, hv_uio_ring_mmap);if (ret) {dev_err (device,"sysfs create ring bin file failed; %d\n" , ret);vmbus_close (new_sc);
@@ -350,10 +337,18 @@ hv_uio_probe(struct hv_device *dev, goto fail_close; } - ret =sysfs_create_bin_file (&channel->kobj, &ring_buffer_bin_attr); -if (ret) -dev_notice (&dev->device, -"sysfs create ring bin file failed; %d\n" , ret); +/* +* This internally calls sysfs_update_group, which returns a non-zero value if it executes +* before sysfs_create_group. This is expected as the 'ring' will be created later in +* vmbus_device_register() -> vmbus_add_channel_kobj(). Thus, no need to check the return +* value and print warning. +* +* Creating/exposing sysfs in driver probe is not encouraged as it can lead to race +* conditions with userspace. For backward compatibility, "ring" sysfs could not be removed +* or decoupled from uio_hv_generic probe. Userspace programs can make use of inotify +* APIs to make sure that ring is created. +*/ +hv_create_ring_sysfs (channel, hv_uio_ring_mmap);hv_set_drvdata (dev, pdata);
@@ -375,7 +370,7 @@ hv_uio_remove(struct hv_device *dev)if (!pdata)return ; -sysfs_remove_bin_file (&dev->channel->kobj, &ring_buffer_bin_attr); +hv_remove_ring_sysfs (dev->channel);uio_unregister_device (&pdata->info);hv_uio_cleanup (dev, pdata);
include/linux/hyperv.h +6 -0
@@ -1002,6 +1002,12 @@ struct vmbus_channel {/* The max size of a packet on this channel */ u32 max_pkt_size; + +/* function to mmap ring buffer memory to the channel's sysfs ring attribute */ +int (*mmap_ring_buffer)(struct vmbus_channel *channel,struct vm_area_struct *vma); + +/* boolean to control visibility of sysfs for ring buffer */ +bool ring_sysfs_visible; };#define lock_requestor (channel, flags) \
Signature · 1 line
2.43.0
Greg Kroah-Hartman
maintainerRe: [PATCH v4 1/2] uio_hv_generic: Fix sysfs creation path for ring buffer
Quoted 39 lines · Naman Jain
On regular bootup, devices get registered to VMBus first, so when
uio_hv_genericdriver for a particular device type is probed, the device is already initialized and added, so sysfs creation inuio_hv_genericprobe works fine. However, when device is removed and brought back, the channel rescinds and device again gets registered to VMBus. However this time, theuio_hv_genericdriver is already registered to probe for that device and in this case sysfs creation is tried before the device's kobject gets initialized completely.Fix this by moving the core logic of sysfs creation for ring buffer, from
uio_hv_genericto HyperV's VMBus driver, where rest of the sysfs attributes for the channels are defined. While doing that, make use of attribute groups and macros, instead of creating sysfs directly, to ensure better error handling and code flow.Problem path:
vmbus_process_offer(new offer comes for the VMBus device)
vmbus_add_channel_workvmbus_device_register |-> device_register | |... | |-> hv_uio_probe | |... | |-> sysfs_create_bin_file (leads to a warning as | primary channel's kobject, which is used to | create sysfs is not yet initialized) |-> kset_create_and_add |-> vmbus_add_channel_kobj (initialization of primary channel's kobject happens later)Above code flow is sequential and the warning is always reproducible in this path.
Fixes 9ab877a6ccf8 ("uio_hv_generic: make ring buffer attribute for primary channel")Suggested-by Saurabh Sengar <[email protected]>Suggested-by Michael Kelley <[email protected]>Signed-off-by Naman Jain <[email protected]>
Quoted 6 lines
--- drivers/hv/hyperv_vmbus.h | 6 ++ drivers/hv/vmbus_drv.c | 110 ++++++++++++++++++++++++++++++++++- drivers/uio/uio_hv_generic.c | 39 ++++++------- include/linux/hyperv.h | 6 ++ 4 files changed, 138 insertions(+), 23 deletions(-)
Always run checkpatch on a patch before submitting it for review :(
Re: [PATCH v4 1/2] uio_hv_generic: Fix sysfs creation path for ring buffer
Quoted 47 lines · Greg Kroah-Hartman
Quoted 39 lines · Naman Jain
On regular bootup, devices get registered to VMBus first, so when
uio_hv_genericdriver for a particular device type is probed, the device is already initialized and added, so sysfs creation inuio_hv_genericprobe works fine. However, when device is removed and brought back, the channel rescinds and device again gets registered to VMBus. However this time, theuio_hv_genericdriver is already registered to probe for that device and in this case sysfs creation is tried before the device's kobject gets initialized completely.Fix this by moving the core logic of sysfs creation for ring buffer, from
uio_hv_genericto HyperV's VMBus driver, where rest of the sysfs attributes for the channels are defined. While doing that, make use of attribute groups and macros, instead of creating sysfs directly, to ensure better error handling and code flow.Problem path:
vmbus_process_offer(new offer comes for the VMBus device)
vmbus_add_channel_workvmbus_device_register |-> device_register | |... | |-> hv_uio_probe | |... | |-> sysfs_create_bin_file (leads to a warning as | primary channel's kobject, which is used to | create sysfs is not yet initialized) |-> kset_create_and_add |-> vmbus_add_channel_kobj (initialization of primary channel's kobject happens later)Above code flow is sequential and the warning is always reproducible in this path.
Fixes 9ab877a6ccf8 ("uio_hv_generic: make ring buffer attribute for primary channel")Suggested-by Saurabh Sengar <[email protected]>Suggested-by Michael Kelley <[email protected]>Signed-off-by Naman Jain <[email protected]>Quoted 6 lines
--- drivers/hv/hyperv_vmbus.h | 6 ++ drivers/hv/vmbus_drv.c | 110 ++++++++++++++++++++++++++++++++++- drivers/uio/uio_hv_generic.c | 39 ++++++------- include/linux/hyperv.h | 6 ++ 4 files changed, 138 insertions(+), 23 deletions(-)
Quoted 2 lines
Always run checkpatch on a patch before submitting it for review :(
Hi Greg,
I verified again and I could not see checkpatch warnings with patch 1,2. There was a warning regarding length of characters for link to previous versions in the cover letter but I ignored it.
I tried on latest linux-next tip with this series fetched from lore using b4. I'm sorry, if I am missing something, but can you please try again.
Thanks,
Naman
Greg Kroah-Hartman
maintainerRe: [PATCH v4 1/2] uio_hv_generic: Fix sysfs creation path for ring buffer
Quoted 53 lines · Naman Jain
Quoted 47 lines · Greg Kroah-Hartman
Quoted 39 lines · Naman Jain
On regular bootup, devices get registered to VMBus first, so when
uio_hv_genericdriver for a particular device type is probed, the device is already initialized and added, so sysfs creation inuio_hv_genericprobe works fine. However, when device is removed and brought back, the channel rescinds and device again gets registered to VMBus. However this time, theuio_hv_genericdriver is already registered to probe for that device and in this case sysfs creation is tried before the device's kobject gets initialized completely.Fix this by moving the core logic of sysfs creation for ring buffer, from
uio_hv_genericto HyperV's VMBus driver, where rest of the sysfs attributes for the channels are defined. While doing that, make use of attribute groups and macros, instead of creating sysfs directly, to ensure better error handling and code flow.Problem path:
vmbus_process_offer(new offer comes for the VMBus device)
vmbus_add_channel_workvmbus_device_register |-> device_register | |... | |-> hv_uio_probe | |... | |-> sysfs_create_bin_file (leads to a warning as | primary channel's kobject, which is used to | create sysfs is not yet initialized) |-> kset_create_and_add |-> vmbus_add_channel_kobj (initialization of primary channel's kobject happens later)Above code flow is sequential and the warning is always reproducible in this path.
Fixes 9ab877a6ccf8 ("uio_hv_generic: make ring buffer attribute for primary channel")Suggested-by Saurabh Sengar <[email protected]>Suggested-by Michael Kelley <[email protected]>Signed-off-by Naman Jain <[email protected]>Quoted 6 lines
--- drivers/hv/hyperv_vmbus.h | 6 ++ drivers/hv/vmbus_drv.c | 110 ++++++++++++++++++++++++++++++++++- drivers/uio/uio_hv_generic.c | 39 ++++++------- include/linux/hyperv.h | 6 ++ 4 files changed, 138 insertions(+), 23 deletions(-)Quoted 2 lines
Always run checkpatch on a patch before submitting it for review :(
Quoted 7 lines
Hi Greg,
I verified again and I could not see checkpatch warnings with patch 1,2. There was a warning regarding length of characters for link to previous versions in the cover letter but I ignored it.I tried on latest linux-next tip with this series fetched from lore using b4. I'm sorry, if I am missing something, but can you please try again.
I've pointed out the first thing that I noticed, thanks.
Re: [PATCH v4 1/2] uio_hv_generic: Fix sysfs creation path for ring buffer
Quoted 59 lines · Greg Kroah-Hartman
Quoted 58 lines · Naman Jain
Quoted 47 lines · Greg Kroah-Hartman
Quoted 39 lines · Naman Jain
On regular bootup, devices get registered to VMBus first, so when
uio_hv_genericdriver for a particular device type is probed, the device is already initialized and added, so sysfs creation inuio_hv_genericprobe works fine. However, when device is removed and brought back, the channel rescinds and device again gets registered to VMBus. However this time, theuio_hv_genericdriver is already registered to probe for that device and in this case sysfs creation is tried before the device's kobject gets initialized completely.Fix this by moving the core logic of sysfs creation for ring buffer, from
uio_hv_genericto HyperV's VMBus driver, where rest of the sysfs attributes for the channels are defined. While doing that, make use of attribute groups and macros, instead of creating sysfs directly, to ensure better error handling and code flow.Problem path:
vmbus_process_offer(new offer comes for the VMBus device)vmbus_add_channel_work vmbus_device_register |-> device_register | |... | |-> hv_uio_probe | |... | |-> sysfs_create_bin_file (leads to a warning as | primary channel's kobject, which is used to | create sysfs is not yet initialized) |-> kset_create_and_add |-> vmbus_add_channel_kobj (initialization of primary channel's kobject happens later)Above code flow is sequential and the warning is always reproducible in this path.
Fixes 9ab877a6ccf8 ("uio_hv_generic: make ring buffer attribute for primary channel")Suggested-by Saurabh Sengar <[email protected]>Suggested-by Michael Kelley <[email protected]>Signed-off-by Naman Jain <[email protected]>Quoted 6 lines
--- drivers/hv/hyperv_vmbus.h | 6 ++ drivers/hv/vmbus_drv.c | 110 ++++++++++++++++++++++++++++++++++- drivers/uio/uio_hv_generic.c | 39 ++++++------- include/linux/hyperv.h | 6 ++ 4 files changed, 138 insertions(+), 23 deletions(-)Quoted 2 lines
Always run checkpatch on a patch before submitting it for review :(
Hi Greg,
I verified again and I could not see checkpatch warnings with patch 1,2. There was a warning regarding length of characters for link to previous versions in the cover letter but I ignored it.
Quoted 5 lines
Quoted 2 lines
I tried on latest linux-next tip with this series fetched from lore using b4. I'm sorry, if I am missing something, but can you please try again.
I've pointed out the first thing that I noticed, thanks.
I'll correct it and send the series again shortly. Thanks for catching this.
Regards,
Naman
Greg Kroah-Hartman
maintainerRe: [PATCH v4 1/2] uio_hv_generic: Fix sysfs creation path for ring buffer
In reply to Naman Jaindrivers/hv/vmbus_drv.c static const struct kobj_type vmbus_chan_ktype = { · line 1936
Quoted 7 lines · Naman Jain
drivers/hv/vmbus_drv.c +7
-0+/* +* Skip updating the sysfs group if the primary channel is not yet initialized and sysfs +* group is not yet created. In those cases, the 'ring' will be created later in +* vmbus_device_register() -> vmbus_add_channel_kobj(). +*/ +if (!primary_channel->device_obj->channels_kset) +return 0 ;
2 spaces after "if"?
Yeah, it's minor, but it made me think that checkpatch had not been run.
thanks,
greg k-h
PATCH v4 2/2 Drivers: hv: Make the sysfs node size for the ring buffer dynamic
In reply to Naman JainThe ring buffer size varies across VMBus channels. The size of sysfs node for the ring buffer is currently hardcoded to 4 MB. Userspace clients either use fstat() or hardcode this size for doing mmap(). To address this, make the sysfs node size dynamic to reflect the actual ring buffer size for each channel. This will ensure that fstat() on ring sysfs node always return the correct size of ring buffer.
--- drivers/hv/vmbus_drv.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
drivers/hv/vmbus_drv.c +10 -1
@@ -1820,7 +1820,6 @@ static struct bin_attribute chan_attr_ring_buffer = { .name = "ring", .mode = 0600, }, - .size = 2 * SZ_2M, .mmap = hv_mmap_ring_buffer_wrapper, }; static struct attribute *vmbus_chan_attrs[] = {
@@ -1880,11 +1879,21 @@ static umode_t vmbus_chan_bin_attr_is_visible(struct kobject *kobj,return attr->attr.mode; } +static size_t vmbus_chan_bin_size (struct kobject *kobj, +const struct bin_attribute *bin_attr,int a) +{ +const struct vmbus_channel *channel = + container_of(kobj, struct vmbus_channel, kobj); + +return channel->ringbuffer_pagecount <<PAGE_SHIFT ; +} +static const struct attribute_group vmbus_chan_group = { .attrs = vmbus_chan_attrs, .bin_attrs = vmbus_chan_bin_attrs, .is_visible = vmbus_chan_attr_is_visible, .is_bin_visible = vmbus_chan_bin_attr_is_visible, + .bin_size = vmbus_chan_bin_size, };static const struct kobj_type vmbus_chan_ktype = {
Signature · 1 line
2.43.0
From: Naman Jain <[email protected]> Sent: Wednesday, April 9, 2025 11:09 PM
Quoted 93 lines
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/[email protected]/
* Addressed Michael's comments regarding handling of return value ofsysfs_update_groupinuio_hv_generic.Changes since v2:
https://lore.kernel.org/all/[email protected]/
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_groupreturn value is handled.Changes since v1:
https://lore.kernel.org/all/[email protected]/
* Fixed race condition in settingchannel->mmap_ring_bufferby
introducing a new variable for visibility of sysfs (addressed Greg's comments)
* Used binary attribute fields instead of regular ones for initializingattribute_group.
* Make size of ring sysfs dynamic based on actual ring buffer's size.
* Preferred to keep mmap function inuio_hv_genericto give more control over ring's mmap functionality, since this is specific touio_hv_genericdriver.
* Remove spurious warning during sysfs creation inuio_hv_genericprobe.
* Added comments in a couple of places.Changes since RFC patch:
https://lore.kernel.org/all/[email protected]/
* Different approach to solve the problem is proposed (credits to Michael Kelley).
* Core logic for sysfs creation moved out ofuio_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/0x10Regards,
NamanNaman Jain (2):
uio_hv_generic: Fix sysfs creation path for ring buffer
Drivers: hv: Make the sysfs node size for the ring buffer dynamic
Quoted 6 lines
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:
Quoted 114 lines · Michael Kelley
From: Naman Jain <[email protected]> Sent: Wednesday, April 9, 2025 11:09 PM
Quoted 93 lines
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/[email protected]/
* Addressed Michael's comments regarding handling of return value ofsysfs_update_groupinuio_hv_generic.Changes since v2:
https://lore.kernel.org/all/[email protected]/
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_groupreturn value is handled.Changes since v1:
https://lore.kernel.org/all/[email protected]/
* Fixed race condition in settingchannel->mmap_ring_bufferby
introducing a new variable for visibility of sysfs (addressed Greg's comments)
* Used binary attribute fields instead of regular ones for initializingattribute_group.
* Make size of ring sysfs dynamic based on actual ring buffer's size.
* Preferred to keep mmap function inuio_hv_genericto give more control over ring's mmap functionality, since this is specific touio_hv_genericdriver.
* Remove spurious warning during sysfs creation inuio_hv_genericprobe.
* Added comments in a couple of places.Changes since RFC patch:
https://lore.kernel.org/all/[email protected]/
* Different approach to solve the problem is proposed (credits to Michael Kelley).
* Core logic for sysfs creation moved out ofuio_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/0x10Regards,
NamanNaman Jain (2):
uio_hv_generic: Fix sysfs creation path for ring buffer
Drivers: hv: Make the sysfs node size for the ring buffer dynamicQuoted 6 lines
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_genericdriver 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 invmbus_device_register(), the "ring" attribute appears with the correct size.
Quoted 8 lines
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 <[email protected]>Tested-by Michael Kelley <[email protected]>
Thank you so much Michael for reviewing and testing this thoroughly. Really appreciate it.
Regards,
Naman