| Message ID | 20201117204224.51230-1-justin.ernst@hpe.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add uv_sysfs platform driver | expand |
Hi, On 11/17/20 9:42 PM, Justin Ernst wrote: > Introduce a new platform driver to gather topology information from UV systems > and expose that information via a sysfs interface at /sys/firmware/sgi_uv/. > > Justin Ernst (5): > x86/platform/uv: Remove existing /sys/firmware/sgi_uv/ interface > x86/platform/uv: Add and export uv_bios_* functions > x86/platform/uv: Add new uv_sysfs platform driver > x86/platform/uv: Update ABI documentation of /sys/firmware/sgi_uv/ > x86/platform/uv: Update MAINTAINERS for uv_sysfs driver So patch 1/1 drops the existing /sys/firmware/sgi_uv/coherence_id /sys/firmware/sgi_uv/partition_id sysfs API, then according to patch 4/5 patch 3/5 reintroduces the /sys/firmware/sgi_uv/partition_id API, but the /sys/firmware/sgi_uv/coherence_id file is gone for ever ? I'm not sure what userspace bits (may) depend on this but without more info this looks like a clear violation of the do not break userspace APIs rule. So, based on this, I have to nack this series in its current state. Now if there is a strong believe there are 0 (not a few, but _zero_) users out there who rely on the /sys/firmware/sgi_uv/coherence_id file then this might be ok. But then there needs to be a technical analysis of why this is ok in the commit message of the patch dropping this sysfs file. Also the commit message of patch 1/5 should mention that /sys/firmware/sgi_uv/partition_id will be re-introduced later through another driver. Regards, Hans > > .../ABI/testing/sysfs-firmware-sgi_uv | 137 ++- > MAINTAINERS | 6 + > arch/x86/include/asm/uv/bios.h | 49 + > arch/x86/include/asm/uv/uv_geo.h | 103 +++ > arch/x86/platform/uv/Makefile | 2 +- > arch/x86/platform/uv/bios_uv.c | 54 ++ > arch/x86/platform/uv/uv_sysfs.c | 63 -- > drivers/platform/x86/Kconfig | 11 + > drivers/platform/x86/Makefile | 3 + > drivers/platform/x86/uv_sysfs.c | 853 ++++++++++++++++++ > 10 files changed, 1202 insertions(+), 79 deletions(-) > create mode 100644 arch/x86/include/asm/uv/uv_geo.h > delete mode 100644 arch/x86/platform/uv/uv_sysfs.c > create mode 100644 drivers/platform/x86/uv_sysfs.c > > > base-commit: 4ef8451b332662d004df269d4cdeb7d9f31419b5 >
> Hi, > > On 11/17/20 9:42 PM, Justin Ernst wrote: > > Introduce a new platform driver to gather topology information from UV systems > > and expose that information via a sysfs interface at /sys/firmware/sgi_uv/. > > > > Justin Ernst (5): > > x86/platform/uv: Remove existing /sys/firmware/sgi_uv/ interface > > x86/platform/uv: Add and export uv_bios_* functions > > x86/platform/uv: Add new uv_sysfs platform driver > > x86/platform/uv: Update ABI documentation of /sys/firmware/sgi_uv/ > > x86/platform/uv: Update MAINTAINERS for uv_sysfs driver > > So patch 1/1 drops the existing > > /sys/firmware/sgi_uv/coherence_id > /sys/firmware/sgi_uv/partition_id > > sysfs API, then according to patch 4/5 patch 3/5 reintroduces > the /sys/firmware/sgi_uv/partition_id API, but the /sys/firmware/sgi_uv/coherence_id > file is gone for ever ? > > I'm not sure what userspace bits (may) depend on this but without more info > this looks like a clear violation of the do not break userspace APIs rule. > > So, based on this, I have to nack this series in its current state. > > Now if there is a strong believe there are 0 (not a few, but _zero_) users > out there who rely on the /sys/firmware/sgi_uv/coherence_id file then this > might be ok. But then there needs to be a technical analysis of why this is > ok in the commit message of the patch dropping this sysfs file. > > Also the commit message of patch 1/5 should mention that > /sys/firmware/sgi_uv/partition_id will be re-introduced later through > another driver. Hello Hans, I will resubmit these patches without the API breakage, reintroducing the coherence_id file in the new driver. Thank you for taking the time to look over my patch set. > > Regards, > > Hans