| Message ID | b2d8d283-36cc-42e8-a8e7-e57e9698a9b5@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Series with core improvements/refactorings | expand |
On Sun, Sep 08, 2024 at 02:08:58PM GMT, Heiner Kallweit wrote: > This undocumented attribute returns a version string which hasn't been > changed for ages. libdrm doesn't use it and I also found no other user. > So I think we can remove it. This file is a part of the ABI. Commit 82d5e73f6b79 ("drm: drop obsolete drm_core.h") replaced variable string with the fixed value that we currently have, but at the same it clearly documented that the file is being preserved for the sake of binary compatibility. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/gpu/drm/drm_sysfs.c | 12 ------------ > 1 file changed, 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > index fb3bbb6ad..49e5faf11 100644 > --- a/drivers/gpu/drm/drm_sysfs.c > +++ b/drivers/gpu/drm/drm_sysfs.c > @@ -126,8 +126,6 @@ static const struct component_ops typec_connector_ops = { > .unbind = typec_connector_unbind, > }; > > -static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810"); > - > /** > * drm_sysfs_init - initialize sysfs helpers > * > @@ -140,19 +138,10 @@ static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810"); > */ > int drm_sysfs_init(void) > { > - int err; > - > drm_class = class_create("drm"); > if (IS_ERR(drm_class)) > return PTR_ERR(drm_class); > > - err = class_create_file(drm_class, &class_attr_version.attr); > - if (err) { > - class_destroy(drm_class); > - drm_class = NULL; > - return err; > - } > - > drm_class->devnode = drm_devnode; > > drm_sysfs_acpi_register(); > @@ -169,7 +158,6 @@ void drm_sysfs_destroy(void) > if (IS_ERR_OR_NULL(drm_class)) > return; > drm_sysfs_acpi_unregister(); > - class_remove_file(drm_class, &class_attr_version.attr); > class_destroy(drm_class); > drm_class = NULL; > } > -- > 2.46.0 > >
On 22.09.2024 16:55, Dmitry Baryshkov wrote: > On Sun, Sep 08, 2024 at 02:08:58PM GMT, Heiner Kallweit wrote: >> This undocumented attribute returns a version string which hasn't been >> changed for ages. libdrm doesn't use it and I also found no other user. >> So I think we can remove it. > > This file is a part of the ABI. Commit 82d5e73f6b79 ("drm: drop obsolete > drm_core.h") replaced variable string with the fixed value that we > currently have, but at the same it clearly documented that the file is > being preserved for the sake of binary compatibility. > The drm version attribute is documented neither under Documentation/gpu nor under Documentation/ABI. So do we really have to consider it part of the ABI? And are you aware of any actual user of this attribute? The author of 82d5e73f6b79 wasn't sure either, and therefore didn't dare to drop the attribute (8 yrs ago). He didn't make any statement that the attribute is actually used. 6.12-rc1 is just out, so we could drop the attribute in linux-next and would have several weeks before the next merge window to find out whether anybody complains. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/gpu/drm/drm_sysfs.c | 12 ------------ >> 1 file changed, 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c >> index fb3bbb6ad..49e5faf11 100644 >> --- a/drivers/gpu/drm/drm_sysfs.c >> +++ b/drivers/gpu/drm/drm_sysfs.c >> @@ -126,8 +126,6 @@ static const struct component_ops typec_connector_ops = { >> .unbind = typec_connector_unbind, >> }; >> >> -static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810"); >> - >> /** >> * drm_sysfs_init - initialize sysfs helpers >> * >> @@ -140,19 +138,10 @@ static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810"); >> */ >> int drm_sysfs_init(void) >> { >> - int err; >> - >> drm_class = class_create("drm"); >> if (IS_ERR(drm_class)) >> return PTR_ERR(drm_class); >> >> - err = class_create_file(drm_class, &class_attr_version.attr); >> - if (err) { >> - class_destroy(drm_class); >> - drm_class = NULL; >> - return err; >> - } >> - >> drm_class->devnode = drm_devnode; >> >> drm_sysfs_acpi_register(); >> @@ -169,7 +158,6 @@ void drm_sysfs_destroy(void) >> if (IS_ERR_OR_NULL(drm_class)) >> return; >> drm_sysfs_acpi_unregister(); >> - class_remove_file(drm_class, &class_attr_version.attr); >> class_destroy(drm_class); >> drm_class = NULL; >> } >> -- >> 2.46.0 >> >> >
On September 30, 2024 1:49:41 PM GMT+03:00, Heiner Kallweit <hkallweit1@gmail.com> wrote: >On 22.09.2024 16:55, Dmitry Baryshkov wrote: >> On Sun, Sep 08, 2024 at 02:08:58PM GMT, Heiner Kallweit wrote: >>> This undocumented attribute returns a version string which hasn't been >>> changed for ages. libdrm doesn't use it and I also found no other user. >>> So I think we can remove it. >> >> This file is a part of the ABI. Commit 82d5e73f6b79 ("drm: drop obsolete >> drm_core.h") replaced variable string with the fixed value that we >> currently have, but at the same it clearly documented that the file is >> being preserved for the sake of binary compatibility. >> >The drm version attribute is documented neither under Documentation/gpu >nor under Documentation/ABI. So do we really have to consider it >part of the ABI? And are you aware of any actual user of this attribute? > >The author of 82d5e73f6b79 wasn't sure either, and therefore didn't >dare to drop the attribute (8 yrs ago). He didn't make any statement that >the attribute is actually used. A very quick search points out that the file is being used: <https://codesearch.debian.net/search?q=drm%2Fversion> > >6.12-rc1 is just out, so we could drop the attribute in linux-next and >would have several weeks before the next merge window to find out >whether anybody complains. No, this is not the way to treat userspace ABI. > >>> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>> --- >>> drivers/gpu/drm/drm_sysfs.c | 12 ------------ >>> 1 file changed, 12 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c >>> index fb3bbb6ad..49e5faf11 100644 >>> --- a/drivers/gpu/drm/drm_sysfs.c >>> +++ b/drivers/gpu/drm/drm_sysfs.c >>> @@ -126,8 +126,6 @@ static const struct component_ops typec_connector_ops = { >>> .unbind = typec_connector_unbind, >>> }; >>> >>> -static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810"); >>> - >>> /** >>> * drm_sysfs_init - initialize sysfs helpers >>> * >>> @@ -140,19 +138,10 @@ static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810"); >>> */ >>> int drm_sysfs_init(void) >>> { >>> - int err; >>> - >>> drm_class = class_create("drm"); >>> if (IS_ERR(drm_class)) >>> return PTR_ERR(drm_class); >>> >>> - err = class_create_file(drm_class, &class_attr_version.attr); >>> - if (err) { >>> - class_destroy(drm_class); >>> - drm_class = NULL; >>> - return err; >>> - } >>> - >>> drm_class->devnode = drm_devnode; >>> >>> drm_sysfs_acpi_register(); >>> @@ -169,7 +158,6 @@ void drm_sysfs_destroy(void) >>> if (IS_ERR_OR_NULL(drm_class)) >>> return; >>> drm_sysfs_acpi_unregister(); >>> - class_remove_file(drm_class, &class_attr_version.attr); >>> class_destroy(drm_class); >>> drm_class = NULL; >>> } >>> -- >>> 2.46.0 >>> >>> >> >
On 01.10.2024 12:01, Dmitry Baryshkov wrote: > On September 30, 2024 1:49:41 PM GMT+03:00, Heiner Kallweit <hkallweit1@gmail.com> wrote: >> On 22.09.2024 16:55, Dmitry Baryshkov wrote: >>> On Sun, Sep 08, 2024 at 02:08:58PM GMT, Heiner Kallweit wrote: >>>> This undocumented attribute returns a version string which hasn't been >>>> changed for ages. libdrm doesn't use it and I also found no other user. >>>> So I think we can remove it. >>> >>> This file is a part of the ABI. Commit 82d5e73f6b79 ("drm: drop obsolete >>> drm_core.h") replaced variable string with the fixed value that we >>> currently have, but at the same it clearly documented that the file is >>> being preserved for the sake of binary compatibility. >>> >> The drm version attribute is documented neither under Documentation/gpu >> nor under Documentation/ABI. So do we really have to consider it >> part of the ABI? And are you aware of any actual user of this attribute? >> >> The author of 82d5e73f6b79 wasn't sure either, and therefore didn't >> dare to drop the attribute (8 yrs ago). He didn't make any statement that >> the attribute is actually used. > > A very quick search points out that the file is being used: > > <https://codesearch.debian.net/search?q=drm%2Fversion> > Thanks. However this script doesn't actually use the version value and would work perfectly fine also w/o this attribute. > >> >> 6.12-rc1 is just out, so we could drop the attribute in linux-next and >> would have several weeks before the next merge window to find out >> whether anybody complains. > > No, this is not the way to treat userspace ABI. > Mileage of subsystem maintainers seems to vary in this regard. See following example where Greg supported such an approach. https://www.spinics.net/lists/linux-i2c/msg71821.html But fine with me, we can also leave the version attribute in. >> >>>> >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>>> --- >>>> drivers/gpu/drm/drm_sysfs.c | 12 ------------ >>>> 1 file changed, 12 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c >>>> index fb3bbb6ad..49e5faf11 100644 >>>> --- a/drivers/gpu/drm/drm_sysfs.c >>>> +++ b/drivers/gpu/drm/drm_sysfs.c >>>> @@ -126,8 +126,6 @@ static const struct component_ops typec_connector_ops = { >>>> .unbind = typec_connector_unbind, >>>> }; >>>> >>>> -static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810"); >>>> - >>>> /** >>>> * drm_sysfs_init - initialize sysfs helpers >>>> * >>>> @@ -140,19 +138,10 @@ static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810"); >>>> */ >>>> int drm_sysfs_init(void) >>>> { >>>> - int err; >>>> - >>>> drm_class = class_create("drm"); >>>> if (IS_ERR(drm_class)) >>>> return PTR_ERR(drm_class); >>>> >>>> - err = class_create_file(drm_class, &class_attr_version.attr); >>>> - if (err) { >>>> - class_destroy(drm_class); >>>> - drm_class = NULL; >>>> - return err; >>>> - } >>>> - >>>> drm_class->devnode = drm_devnode; >>>> >>>> drm_sysfs_acpi_register(); >>>> @@ -169,7 +158,6 @@ void drm_sysfs_destroy(void) >>>> if (IS_ERR_OR_NULL(drm_class)) >>>> return; >>>> drm_sysfs_acpi_unregister(); >>>> - class_remove_file(drm_class, &class_attr_version.attr); >>>> class_destroy(drm_class); >>>> drm_class = NULL; >>>> } >>>> -- >>>> 2.46.0 >>>> >>>> >>> >> > >
On October 1, 2024 2:07:28 PM GMT+03:00, Heiner Kallweit <hkallweit1@gmail.com> wrote: >On 01.10.2024 12:01, Dmitry Baryshkov wrote: >> On September 30, 2024 1:49:41 PM GMT+03:00, Heiner Kallweit <hkallweit1@gmail.com> wrote: >>> On 22.09.2024 16:55, Dmitry Baryshkov wrote: >>>> On Sun, Sep 08, 2024 at 02:08:58PM GMT, Heiner Kallweit wrote: >>>>> This undocumented attribute returns a version string which hasn't been >>>>> changed for ages. libdrm doesn't use it and I also found no other user. >>>>> So I think we can remove it. >>>> >>>> This file is a part of the ABI. Commit 82d5e73f6b79 ("drm: drop obsolete >>>> drm_core.h") replaced variable string with the fixed value that we >>>> currently have, but at the same it clearly documented that the file is >>>> being preserved for the sake of binary compatibility. >>>> >>> The drm version attribute is documented neither under Documentation/gpu >>> nor under Documentation/ABI. So do we really have to consider it >>> part of the ABI? And are you aware of any actual user of this attribute? >>> >>> The author of 82d5e73f6b79 wasn't sure either, and therefore didn't >>> dare to drop the attribute (8 yrs ago). He didn't make any statement that >>> the attribute is actually used. >> >> A very quick search points out that the file is being used: >> >> <https://codesearch.debian.net/search?q=drm%2Fversion> >> > >Thanks. However this script doesn't actually use the version value >and would work perfectly fine also w/o this attribute. > >> >>> >>> 6.12-rc1 is just out, so we could drop the attribute in linux-next and >>> would have several weeks before the next merge window to find out >>> whether anybody complains. >> >> No, this is not the way to treat userspace ABI. >> > >Mileage of subsystem maintainers seems to vary in this regard. >See following example where Greg supported such an approach. >https://www.spinics.net/lists/linux-i2c/msg71821.html >But fine with me, we can also leave the version attribute in. Let's leave the final decision to subsystem maintainers. > >>> >>>>> >>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>>>> --- >>>>> drivers/gpu/drm/drm_sysfs.c | 12 ------------ >>>>> 1 file changed, 12 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c >>>>> index fb3bbb6ad..49e5faf11 100644 >>>>> --- a/drivers/gpu/drm/drm_sysfs.c >>>>> +++ b/drivers/gpu/drm/drm_sysfs.c >>>>> @@ -126,8 +126,6 @@ static const struct component_ops typec_connector_ops = { >>>>> .unbind = typec_connector_unbind, >>>>> }; >>>>> >>>>> -static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810"); >>>>> - >>>>> /** >>>>> * drm_sysfs_init - initialize sysfs helpers >>>>> * >>>>> @@ -140,19 +138,10 @@ static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810"); >>>>> */ >>>>> int drm_sysfs_init(void) >>>>> { >>>>> - int err; >>>>> - >>>>> drm_class = class_create("drm"); >>>>> if (IS_ERR(drm_class)) >>>>> return PTR_ERR(drm_class); >>>>> >>>>> - err = class_create_file(drm_class, &class_attr_version.attr); >>>>> - if (err) { >>>>> - class_destroy(drm_class); >>>>> - drm_class = NULL; >>>>> - return err; >>>>> - } >>>>> - >>>>> drm_class->devnode = drm_devnode; >>>>> >>>>> drm_sysfs_acpi_register(); >>>>> @@ -169,7 +158,6 @@ void drm_sysfs_destroy(void) >>>>> if (IS_ERR_OR_NULL(drm_class)) >>>>> return; >>>>> drm_sysfs_acpi_unregister(); >>>>> - class_remove_file(drm_class, &class_attr_version.attr); >>>>> class_destroy(drm_class); >>>>> drm_class = NULL; >>>>> } >>>>> -- >>>>> 2.46.0 >>>>> >>>>> >>>> >>> >> >> >
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index fb3bbb6ad..49e5faf11 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -126,8 +126,6 @@ static const struct component_ops typec_connector_ops = { .unbind = typec_connector_unbind, }; -static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810"); - /** * drm_sysfs_init - initialize sysfs helpers * @@ -140,19 +138,10 @@ static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810"); */ int drm_sysfs_init(void) { - int err; - drm_class = class_create("drm"); if (IS_ERR(drm_class)) return PTR_ERR(drm_class); - err = class_create_file(drm_class, &class_attr_version.attr); - if (err) { - class_destroy(drm_class); - drm_class = NULL; - return err; - } - drm_class->devnode = drm_devnode; drm_sysfs_acpi_register(); @@ -169,7 +158,6 @@ void drm_sysfs_destroy(void) if (IS_ERR_OR_NULL(drm_class)) return; drm_sysfs_acpi_unregister(); - class_remove_file(drm_class, &class_attr_version.attr); class_destroy(drm_class); drm_class = NULL; }
This undocumented attribute returns a version string which hasn't been changed for ages. libdrm doesn't use it and I also found no other user. So I think we can remove it. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/gpu/drm/drm_sysfs.c | 12 ------------ 1 file changed, 12 deletions(-)