[Question] orphan platform data header

[Question] orphan platform data header

[Masahiro Yamada]

Hi.

I see several platform-data headers
that are not used in upstream.


For instance, please look at this driver:
drivers/leds/leds-netxbig.c

If I understood it correctly, this driver
supports both device tree and legacy board-file.


I grepped 'netxbig_led_platform_data', but
I only found the driver and platform_data header.
No board-file in upstream.

masahiro@grover:~/ref/linux$ git grep netxbig_led_platform_data
drivers/leds/leds-netxbig.c:                          struct
netxbig_led_platform_data *pdata,
drivers/leds/leds-netxbig.c:                                 struct
netxbig_led_platform_data *pdata)
drivers/leds/leds-netxbig.c:                      struct
netxbig_led_platform_data *pdata)
drivers/leds/leds-netxbig.c:    struct netxbig_led_platform_data
*pdata = dev_get_platdata(&pdev->dev);
include/linux/platform_data/leds-kirkwood-netxbig.h:struct
netxbig_led_platform_data {



So, what shall we do?

Drop the board-file support? Or, keep it
in case somebody is still using their board-files
in downstream?




--
Best Regards
Masahiro Yamada

Re: [Question] orphan platform data header

[Arnd Bergmann]

On Sat, Jul 20, 2019 at 5:26 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> masahiro@grover:~/ref/linux$ git grep netxbig_led_platform_data
> drivers/leds/leds-netxbig.c:                          struct
> netxbig_led_platform_data *pdata,
> drivers/leds/leds-netxbig.c:                                 struct
> netxbig_led_platform_data *pdata)
> drivers/leds/leds-netxbig.c:                      struct
> netxbig_led_platform_data *pdata)
> drivers/leds/leds-netxbig.c:    struct netxbig_led_platform_data
> *pdata = dev_get_platdata(&pdev->dev);
> include/linux/platform_data/leds-kirkwood-netxbig.h:struct
> netxbig_led_platform_data {
>
>
>
> So, what shall we do?
>
> Drop the board-file support? Or, keep it
> in case somebody is still using their board-files
> in downstream?

Generally speaking, I'd remove the board file support in another
case like this, but it's worth looking at when it was last used and by
what.

For this file, all boards got converted to DT, and the old setup
code removed in commit ebc278f15759 ("ARM: mvebu: remove static
LED setup for netxbig boards"), four years ago, so it's a fairly
easy decision to make it DT only.

      Arnd

Re: [Question] orphan platform data header

[Masahiro Yamada]

Hi Arnd,

On Sat, Jul 20, 2019 at 10:55 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Sat, Jul 20, 2019 at 5:26 AM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> >
> > masahiro@grover:~/ref/linux$ git grep netxbig_led_platform_data
> > drivers/leds/leds-netxbig.c:                          struct
> > netxbig_led_platform_data *pdata,
> > drivers/leds/leds-netxbig.c:                                 struct
> > netxbig_led_platform_data *pdata)
> > drivers/leds/leds-netxbig.c:                      struct
> > netxbig_led_platform_data *pdata)
> > drivers/leds/leds-netxbig.c:    struct netxbig_led_platform_data
> > *pdata = dev_get_platdata(&pdev->dev);
> > include/linux/platform_data/leds-kirkwood-netxbig.h:struct
> > netxbig_led_platform_data {
> >
> >
> >
> > So, what shall we do?
> >
> > Drop the board-file support? Or, keep it
> > in case somebody is still using their board-files
> > in downstream?
>
> Generally speaking, I'd remove the board file support in another
> case like this, but it's worth looking at when it was last used and by
> what.
>
> For this file, all boards got converted to DT, and the old setup
> code removed in commit ebc278f15759 ("ARM: mvebu: remove static
> LED setup for netxbig boards"), four years ago, so it's a fairly
> easy decision to make it DT only.


Thanks.

I see another case, which is difficult
to make a decision.

For example, drivers/spi/spi-tle62x0.c

This driver supports only board-file, but the board-file
is not found in upstream.

Unless I am terribly missing something,
there is no one who passes tle62x0_pdata
to this driver.

$ git grep tle62x0_pdata
drivers/spi/spi-tle62x0.c:      struct tle62x0_pdata *pdata;
include/linux/spi/tle62x0.h:struct tle62x0_pdata {

But, removing board-file support
makes this driver completely useless...


--
Best Regards
Masahiro Yamada

Re: [Question] orphan platform data header

[Arnd Bergmann]

On Sun, Jul 21, 2019 at 5:45 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> On Sat, Jul 20, 2019 at 10:55 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Sat, Jul 20, 2019 at 5:26 AM Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> > > So, what shall we do?
> > >
> > > Drop the board-file support? Or, keep it
> > > in case somebody is still using their board-files
> > > in downstream?
>>
> > For this file, all boards got converted to DT, and the old setup
> > code removed in commit ebc278f15759 ("ARM: mvebu: remove static
> > LED setup for netxbig boards"), four years ago, so it's a fairly
> > easy decision to make it DT only.
>
> I see another case, which is difficult
> to make a decision.
>
> For example, drivers/spi/spi-tle62x0.c
>
> This driver supports only board-file, but the board-file
> is not found in upstream.
>
> Unless I am terribly missing something,
> there is no one who passes tle62x0_pdata
> to this driver.
>
> $ git grep tle62x0_pdata
> drivers/spi/spi-tle62x0.c:      struct tle62x0_pdata *pdata;
> include/linux/spi/tle62x0.h:struct tle62x0_pdata {
>
> But, removing board-file support
> makes this driver completely useless...

Adding Ben Dooks to Cc.

I suspect this driver is completely obsolete and should be removed.

For some reason, it's not an SPI controller driver like all the other
files in that directory, but implements low-level access to the state
of a particular SPI device.

However, there should not really be a low-level driver for it that
just exports the pins to user space. It should either be a gpiolib
driver to let other drivers talk to the pins, or a high-level driver that
exposes the intended functionality (watchdog, regulator, ...)
to those respective subsystems.

       Arnd

Re: [Question] orphan platform data header

[Masahiro Yamada]

On Sun, Jul 21, 2019 at 6:10 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Sun, Jul 21, 2019 at 5:45 AM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> > On Sat, Jul 20, 2019 at 10:55 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Sat, Jul 20, 2019 at 5:26 AM Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> > > > So, what shall we do?
> > > >
> > > > Drop the board-file support? Or, keep it
> > > > in case somebody is still using their board-files
> > > > in downstream?
> >>
> > > For this file, all boards got converted to DT, and the old setup
> > > code removed in commit ebc278f15759 ("ARM: mvebu: remove static
> > > LED setup for netxbig boards"), four years ago, so it's a fairly
> > > easy decision to make it DT only.
> >
> > I see another case, which is difficult
> > to make a decision.
> >
> > For example, drivers/spi/spi-tle62x0.c
> >
> > This driver supports only board-file, but the board-file
> > is not found in upstream.
> >
> > Unless I am terribly missing something,
> > there is no one who passes tle62x0_pdata
> > to this driver.
> >
> > $ git grep tle62x0_pdata
> > drivers/spi/spi-tle62x0.c:      struct tle62x0_pdata *pdata;
> > include/linux/spi/tle62x0.h:struct tle62x0_pdata {
> >
> > But, removing board-file support
> > makes this driver completely useless...
>
> Adding Ben Dooks to Cc.
>
> I suspect this driver is completely obsolete and should be removed.
>
> For some reason, it's not an SPI controller driver like all the other
> files in that directory, but implements low-level access to the state
> of a particular SPI device.
>
> However, there should not really be a low-level driver for it that
> just exports the pins to user space. It should either be a gpiolib
> driver to let other drivers talk to the pins, or a high-level driver that
> exposes the intended functionality (watchdog, regulator, ...)
> to those respective subsystems.
>
>        Arnd


Another example that I have no idea
how it works:

drivers/net/hamradio/yam.c

yam_ioctl() reads data from user-space,
but the data structures for ioctl are
defined in include/linux/yam.h

This header is not exported to user-space
since it is outside of the uapi directory.

I dug the git history, but it has never
exported to user-space in the past.

I do not know how user-space programs can
pass-in data to the kernel.

If we want to fix this, we could move it
to include/uapi/linux/yam.h

But, if nobody has reported any problem about this,
it might be a good proof that nobody is using this driver.

Maybe, can we simply drop odd drivers??


-- 
Best Regards
Masahiro Yamada

Re: [Question] orphan platform data header

[Arnd Bergmann]

On Sun, Jul 21, 2019 at 2:13 PM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> On Sun, Jul 21, 2019 at 6:10 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Sun, Jul 21, 2019 at 5:45 AM Masahiro Yamada
> > <yamada.masahiro@socionext.com> wrote:
> > > On Sat, Jul 20, 2019 at 10:55 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > On Sat, Jul 20, 2019 at 5:26 AM Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>
> Another example that I have no idea
> how it works:
>
> drivers/net/hamradio/yam.c
>
> yam_ioctl() reads data from user-space,
> but the data structures for ioctl are
> defined in include/linux/yam.h

That is different: the hardware attaches to a serial port and may well
be usable, and the user space side just contains a copy of the header,
see https://github.com/nwdigitalradio/ax25-tools/tree/master/yamdrv

> If we want to fix this, we could move it
> to include/uapi/linux/yam.h

We could do that, or just leave it alone, as nobody would
tell the difference.

> But, if nobody has reported any problem about this,
> it might be a good proof that nobody is using this driver.
>
> Maybe, can we simply drop odd drivers??

Both the kernel driver and the user space side have a maintainer, and
I see no indication that it is actually broken. The driver is clearly
a relic from old times (earlier than 2.4) and we would not merge
this driver today.

It seems more useful to keep looking for drivers with a platform_data
header file that is no longer included by any platform for candidates
that may be obsolete.

        Arnd

Re: [Question] orphan platform data header

[Masahiro Yamada]

On Sun, Jul 21, 2019 at 11:15 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Sun, Jul 21, 2019 at 2:13 PM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> > On Sun, Jul 21, 2019 at 6:10 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Sun, Jul 21, 2019 at 5:45 AM Masahiro Yamada
> > > <yamada.masahiro@socionext.com> wrote:
> > > > On Sat, Jul 20, 2019 at 10:55 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > > On Sat, Jul 20, 2019 at 5:26 AM Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >
> >
> > Another example that I have no idea
> > how it works:
> >
> > drivers/net/hamradio/yam.c
> >
> > yam_ioctl() reads data from user-space,
> > but the data structures for ioctl are
> > defined in include/linux/yam.h
>
> That is different: the hardware attaches to a serial port and may well
> be usable, and the user space side just contains a copy of the header,
> see https://github.com/nwdigitalradio/ax25-tools/tree/master/yamdrv

Oh, I did not know that
user-space had a copy of that.


> > If we want to fix this, we could move it
> > to include/uapi/linux/yam.h
>
> We could do that, or just leave it alone, as nobody would
> tell the difference.

When we are changing structures in uapi,
it is very likely a red alert.
On the other hand, we can change code outside of uapi
more safely.
One benefit of uapi is we can catch the compatibility level
from the directory path.


>
> > But, if nobody has reported any problem about this,
> > it might be a good proof that nobody is using this driver.
> >
> > Maybe, can we simply drop odd drivers??
>
> Both the kernel driver and the user space side have a maintainer, and
> I see no indication that it is actually broken. The driver is clearly
> a relic from old times (earlier than 2.4) and we would not merge
> this driver today.
>
> It seems more useful to keep looking for drivers with a platform_data
> header file that is no longer included by any platform for candidates
> that may be obsolete.

OK.

Thanks.




-- 
Best Regards
Masahiro Yamada

Re: [Question] orphan platform data header

[Enrico Weigelt, metux IT consult]

On 21.07.19 16:15, Arnd Bergmann wrote:

> That is different: the hardware attaches to a serial port and may well
> be usable, and the user space side just contains a copy of the header,
> see https://github.com/nwdigitalradio/ax25-tools/tree/master/yamdrv

I believe that such header copies in userland applications are
conceptionally wrong. Whenever something changes, both sides need
to be kept in sync.

Maybe we should talk to the hamradio folks to get this cleaned up.
IMHO, this header should go to uapi.

> It seems more useful to keep looking for drivers with a platform_data
> header file that is no longer included by any platform for candidates
> that may be obsolete.

Some folks see platform_data old legacy that should be removed, but I
don't aggree. For example w/ apu2 board driver (and corresponding
amd-fch-gpio driver) I even had to introduce a pdata struct, so the
board driver could configure the gpio driver. Certainly, I would have
preferred doing everything via DT, but that's not available on x86/acpi
targets (if anybody knows a way to inject a DT snippet just for one
driver in such a scenario, please let me know).


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [Question] orphan platform data header

[Arnd Bergmann]

On Mon, Jul 22, 2019 at 8:46 AM Enrico Weigelt, metux IT consult
<lkml@metux.net> wrote:
> On 21.07.19 16:15, Arnd Bergmann wrote:
>
> > That is different: the hardware attaches to a serial port and may well
> > be usable, and the user space side just contains a copy of the header,
> > see https://github.com/nwdigitalradio/ax25-tools/tree/master/yamdrv
>
> I believe that such header copies in userland applications are
> conceptionally wrong. Whenever something changes, both sides need
> to be kept in sync.
>
> Maybe we should talk to the hamradio folks to get this cleaned up.
> IMHO, this header should go to uapi.

Having copies of driver specific uapi headers is rather common,
and you won't have much success trying to get everyone to change
that.

The reasons why those applications do it are:

- The kernel already gives ABI guarantees so anything built with an
   old header file is expected to keep working indefinitely.
- Using a new header file won't help unless the application also
   knows about the added interfaces
- If an application uses more recent additions to the kernel headers,
  it either has to have a matching version of that header, or use a long
  series of #ifdef checks to deal with arbitrary versions.

> > It seems more useful to keep looking for drivers with a platform_data
> > header file that is no longer included by any platform for candidates
> > that may be obsolete.
>
> Some folks see platform_data old legacy that should be removed, but I
> don't aggree. For example w/ apu2 board driver (and corresponding
> amd-fch-gpio driver) I even had to introduce a pdata struct, so the
> board driver could configure the gpio driver.

The case that we are interested in is drivers that previously used
platform data in legacy board files that have since been replaced
with dtb based boots.

> Certainly, I would have
> preferred doing everything via DT, but that's not available on x86/acpi
> targets (if anybody knows a way to inject a DT snippet just for one
> driver in such a scenario, please let me know).

It's been done before, but usually with overlays that don't necessarily
make it any better than platform data. If you have a set of drivers where
one of them creates a platform device for the other driver, then platform
data is usually the easiest way, and I'm not aware of any move to
get rid of that.

As an alternative, you can use the generalized property support
from include/linux/property.h that works on top of DT, ACPI or
plain platform devices.

        Arnd