ioctl number change / backwards compatibility doubt

ioctl number change / backwards compatibility doubt

[Paulo Miguel Almeida]

Hi everyone,

Context:

I've been working on a driver called pi433 in the staging area and it 
basically exposes a char device so the user can read/write stuff to
it while obtaining tx/rx configuration via ioctl calls.

Tx/Rx configurations are both structs that (ideally) would be exposed
to the userspace to let the developer to #include it on their code.

Info that *might* be relevant about this driver:

- This driver went straight to the staging area due to a few TODOs
  listed by the original author. 
- The ioctl Code and Seq numbers are conflicting with the ones listed
  in the ioctl-numbers.rst

Problem:

I realized that one of the structs used to pass/retrieve info needs
to have some of its members changed (data type and etc)

Questions:

1: Given the driver's history and ioctl number conflit, is the backwards
compatibility something to be kept or not to be taken into consideration
as ioctl numbering rules weren't followed?

2: The original author lists on the TODO file of the driver that 'he is
afraid that using ioctl wasn't a good idea'. I pondered the alternatives
and, *in case I can get rid of ioctl*, sysfs || configfs could be used. Does
anyone suggests a different approach?

Best regards,

Paulo Almeida


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: ioctl number change / backwards compatibility doubt

[Greg KH]

On Mon, Jan 17, 2022 at 08:01:25PM +1300, Paulo Miguel Almeida wrote:
> Hi everyone,
> 
> Context:
> 
> I've been working on a driver called pi433 in the staging area and it 
> basically exposes a char device so the user can read/write stuff to
> it while obtaining tx/rx configuration via ioctl calls.
> 
> Tx/Rx configurations are both structs that (ideally) would be exposed
> to the userspace to let the developer to #include it on their code.
> 
> Info that *might* be relevant about this driver:
> 
> - This driver went straight to the staging area due to a few TODOs
>   listed by the original author. 
> - The ioctl Code and Seq numbers are conflicting with the ones listed
>   in the ioctl-numbers.rst

Not many people ever look at that file, and it is ok to have conflicts
as the same tool should never have to handle multiple drivers where a
conflict happens.

> Problem:
> 
> I realized that one of the structs used to pass/retrieve info needs
> to have some of its members changed (data type and etc)

Great!

> Questions:
> 
> 1: Given the driver's history and ioctl number conflit, is the backwards
> compatibility something to be kept or not to be taken into consideration
> as ioctl numbering rules weren't followed?

Try to find out who is using these ioctls.  If you can change the
userspace tool at the same time, all is fine.  If not, then there can be
problems.

> 2: The original author lists on the TODO file of the driver that 'he is
> afraid that using ioctl wasn't a good idea'. I pondered the alternatives
> and, *in case I can get rid of ioctl*, sysfs || configfs could be used. Does
> anyone suggests a different approach?

Same answer as above, it depends on what userspace tool is using these
ioctls.  Also it depends on what they do.  Many informational ioctls can
just be replaced with sysfs files, and many configuration ioctls can be
replaced with configfs, but for other things, sometimes you need an
ioctl.

So it depends.  Try to get ahold of the userspace side and then you can
usually work it out.

thanks,

greg k-h

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: ioctl number change / backwards compatibility doubt

[Paulo Miguel Almeida]

On Mon, Jan 17, 2022 at 01:58:42PM +0100, Greg KH wrote:
> Not many people ever look at that file, and it is ok to have conflicts
> as the same tool should never have to handle multiple drivers where a
> conflict happens.

Noted

> > 1: Given the driver's history and ioctl number conflit, is the backwards
> > compatibility something to be kept or not to be taken into consideration
> > as ioctl numbering rules weren't followed?
> 
> Try to find out who is using these ioctls.  If you can change the
> userspace tool at the same time, all is fine.  If not, then there can be
> problems.

Apologies for the delay, I had emailed the original author and I was waiting 
for his reply before I could answer this. It turns out I haven't gotten
an official answer from him yet. (I do understand that he might be busy)

I googled a fair bit of time and I'm 99% confident that there isn't such
userspace/lib tool so I guess this will have done the hard way :(

> > 2: The original author lists on the TODO file of the driver that 'he is
> > afraid that using ioctl wasn't a good idea'. I pondered the alternatives
> > and, *in case I can get rid of ioctl*, sysfs || configfs could be used. Does
> > anyone suggests a different approach?
> 
> Same answer as above, it depends on what userspace tool is using these
> ioctls.  Also it depends on what they do.  Many informational ioctls can
> just be replaced with sysfs files, and many configuration ioctls can be
> replaced with configfs, but for other things, sometimes you need an
> ioctl.
> 
> So it depends.  Try to get ahold of the userspace side and then you can
> usually work it out.
> 

Dan Carpenter suggested in one of patch reviews that we keep the ioctl
for backwards compatibility but start a brand new sysfs implementation to
encompass existing functionality to make it easier to add new features 
in future.

I will wrap my head around it and send some RFC patches soon.

thanks,

Paulo Almeida


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: ioctl number change / backwards compatibility doubt

[Greg KH]

On Sun, Jan 23, 2022 at 08:55:30PM +1300, Paulo Miguel Almeida wrote:
> > > 1: Given the driver's history and ioctl number conflit, is the backwards
> > > compatibility something to be kept or not to be taken into consideration
> > > as ioctl numbering rules weren't followed?
> > 
> > Try to find out who is using these ioctls.  If you can change the
> > userspace tool at the same time, all is fine.  If not, then there can be
> > problems.
> 
> Apologies for the delay, I had emailed the original author and I was waiting 
> for his reply before I could answer this. It turns out I haven't gotten
> an official answer from him yet. (I do understand that he might be busy)
> 
> I googled a fair bit of time and I'm 99% confident that there isn't such
> userspace/lib tool so I guess this will have done the hard way :(

If there is no tool, why was the ioctl code written at all?  Something
had to call it.


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: ioctl number change / backwards compatibility doubt

[Paulo Miguel Almeida]

On Sun, Jan 23, 2022 at 12:04:48PM +0100, Greg KH wrote:
> On Sun, Jan 23, 2022 at 08:55:30PM +1300, Paulo Miguel Almeida wrote:
> > 
> > I googled a fair bit of time and I'm 99% confident that there isn't such
> > userspace/lib tool so I guess this will have done the hard way :(
> 
> If there is no tool, why was the ioctl code written at all?  Something
> had to call it.
> 

when you told me to look for the userspace tool that interfaced with the
ioctl, my interpretation was that you were referring to something akin
to what /usr/bin/uname utility is to the syscall uname. Please correct me 
if I'm wrong.

re: what calls the ioctl created by the driver.

I'm led to believe that users of this driver make ioctl sycall
invocations directly from their application's source code like this:

#include "pi433_if.h"		/* userspace driver header */
#include <sys/ioctl.h>		/* ioctl */

int file_desc = open("/dev/pi433.0", O_RDWR);
struct pi433_tx_cfg tx_cfg = {
	.frequency = 433000000,
	.bit_rate = 4800,
	<omitted for brevity>...
};

int ret_val = ioctl(file_desc, PI433_IOC_WR_TX_CFG, tx_cfg);
....

thanks,

Paulo Almeida


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: ioctl number change / backwards compatibility doubt

[Greg KH]

On Mon, Jan 24, 2022 at 05:49:06PM +1300, Paulo Miguel Almeida wrote:
> On Sun, Jan 23, 2022 at 12:04:48PM +0100, Greg KH wrote:
> > On Sun, Jan 23, 2022 at 08:55:30PM +1300, Paulo Miguel Almeida wrote:
> > > 
> > > I googled a fair bit of time and I'm 99% confident that there isn't such
> > > userspace/lib tool so I guess this will have done the hard way :(
> > 
> > If there is no tool, why was the ioctl code written at all?  Something
> > had to call it.
> > 
> 
> when you told me to look for the userspace tool that interfaced with the
> ioctl, my interpretation was that you were referring to something akin
> to what /usr/bin/uname utility is to the syscall uname. Please correct me 
> if I'm wrong.
> 
> re: what calls the ioctl created by the driver.
> 
> I'm led to believe that users of this driver make ioctl sycall
> invocations directly from their application's source code like this:
> 
> #include "pi433_if.h"		/* userspace driver header */
> #include <sys/ioctl.h>		/* ioctl */
> 
> int file_desc = open("/dev/pi433.0", O_RDWR);
> struct pi433_tx_cfg tx_cfg = {
> 	.frequency = 433000000,
> 	.bit_rate = 4800,
> 	<omitted for brevity>...
> };
> 
> int ret_val = ioctl(file_desc, PI433_IOC_WR_TX_CFG, tx_cfg);
> ....

Yes, sorry for the confusion, this is what I am referring to.  Where is
that userspace code as that is the code you will be needing to change if
you want to change this ioctl interface.

thanks,

greg k-h

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: ioctl number change / backwards compatibility doubt

[Paulo Miguel Almeida]

On Mon, Jan 24, 2022 at 07:20:45AM +0100, Greg KH wrote:
> On Mon, Jan 24, 2022 at 05:49:06PM +1300, Paulo Miguel Almeida wrote:
> > On Sun, Jan 23, 2022 at 12:04:48PM +0100, Greg KH wrote:
> > 
> > when you told me to look for the userspace tool that interfaced with the
> > ioctl, my interpretation was that you were referring to something akin
> > to what /usr/bin/uname utility is to the syscall uname. Please correct me 
> > if I'm wrong.
> > 
> > re: what calls the ioctl created by the driver.
> > 
> > I'm led to believe that users of this driver make ioctl sycall
> > invocations directly from their application's source code like this:
> > 
> > #include "pi433_if.h"		/* userspace driver header */
> > #include <sys/ioctl.h>		/* ioctl */
> > 
> > int file_desc = open("/dev/pi433.0", O_RDWR);
> > struct pi433_tx_cfg tx_cfg = {
> > 	.frequency = 433000000,
> > 	.bit_rate = 4800,
> > 	<omitted for brevity>...
> > };
> > 
> > int ret_val = ioctl(file_desc, PI433_IOC_WR_TX_CFG, tx_cfg);
> > ....
> 
> Yes, sorry for the confusion, this is what I am referring to.  Where is
> that userspace code as that is the code you will be needing to change if
> you want to change this ioctl interface.
> 
> thanks,
> 
> greg k-h

Hi Greg, 

The original author replied my email with that question.  He sent me
some the code used to interface with char device, however, the source
code he provided me is already incompatible with the current version of
the driver:

#include "rfxx.h" (file header name has changed)

int main(int argc, char *argv[])
{
	...
	struct rfxx_send_config sendConfig; (struct name has changed)
	...
        fd = open("/dev/rf69_0.0", O_RDWR); *(char device name changed)*
	...
	sendConfig.data_mode = packet; *(property doesn't exist)*
        ...
	(IOCTL call has a different name)
	ret = ioctl(fd, RFXX_IOC_WR_SEND_CONFIG, &sendConfig);
        if (ret == -1)
	...
}

Assuming that I tweak these tools he provided me with and publish them,
will I then be able to tweak the structures passed back and forth via
ioctl? (do I need to change ioctl number in this case?) 

The reason why I'm asking this is because given the fact that other
developers could have written similar code for interfacing with the
driver (and that we will never know because code is proprietary), I
won't be sure that I've changed all occurences at the same time, right?

All in all, please correct if there are gaps in this train of thought.

- If a change doesn't break *compiled* code (such as struct renaming) 
then it's 'okay' to make the change ? (this has happened in the this
driver before)

Best regards,

Paulo Almeida


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: ioctl number change / backwards compatibility doubt

[Rogério Valentim Feitoza da Silva]

[-- Attachment #1.1: Type: text/plain, Size: 3324 bytes --]

On Friday, 11 March 2022, Paulo Miguel Almeida <
paulo.miguel.almeida.rodenas@gmail.com> wrote:

> On Mon, Jan 24, 2022 at 07:20:45AM +0100, Greg KH wrote:
> > On Mon, Jan 24, 2022 at 05:49:06PM +1300, Paulo Miguel Almeida wrote:
> > > On Sun, Jan 23, 2022 at 12:04:48PM +0100, Greg KH wrote:
> > >
> > > when you told me to look for the userspace tool that interfaced with
> the
> > > ioctl, my interpretation was that you were referring to something akin
> > > to what /usr/bin/uname utility is to the syscall uname. Please correct
> me
> > > if I'm wrong.
> > >
> > > re: what calls the ioctl created by the driver.
> > >
> > > I'm led to believe that users of this driver make ioctl sycall
> > > invocations directly from their application's source code like this:
> > >
> > > #include "pi433_if.h"               /* userspace driver header */
> > > #include <sys/ioctl.h>              /* ioctl */
> > >
> > > int file_desc = open("/dev/pi433.0", O_RDWR);
> > > struct pi433_tx_cfg tx_cfg = {
> > >     .frequency = 433000000,
> > >     .bit_rate = 4800,
> > >     <omitted for brevity>...
> > > };
> > >
> > > int ret_val = ioctl(file_desc, PI433_IOC_WR_TX_CFG, tx_cfg);
> > > ....
> >
> > Yes, sorry for the confusion, this is what I am referring to.  Where is
> > that userspace code as that is the code you will be needing to change if
> > you want to change this ioctl interface.
> >
> > thanks,
> >
> > greg k-h
>
> Hi Greg,
>
> The original author replied my email with that question.  He sent me
> some the code used to interface with char device, however, the source
> code he provided me is already incompatible with the current version of
> the driver:
>
> #include "rfxx.h" (file header name has changed)
>
> int main(int argc, char *argv[])
> {
>         ...
>         struct rfxx_send_config sendConfig; (struct name has changed)
>         ...
>         fd = open("/dev/rf69_0.0", O_RDWR); *(char device name changed)*
>         ...
>         sendConfig.data_mode = packet; *(property doesn't exist)*
>         ...
>         (IOCTL call has a different name)
>         ret = ioctl(fd, RFXX_IOC_WR_SEND_CONFIG, &sendConfig);
>         if (ret == -1)
>         ...
> }
>
> Assuming that I tweak these tools he provided me with and publish them,
> will I then be able to tweak the structures passed back and forth via
> ioctl? (do I need to change ioctl number in this case?)
>
> The reason why I'm asking this is because given the fact that other
> developers could have written similar code for interfacing with the
> driver (and that we will never know because code is proprietary), I
> won't be sure that I've changed all occurences at the same time, right?
>
> All in all, please correct if there are gaps in this train of thought.
>
> - If a change doesn't break *compiled* code (such as struct renaming)
> then it's 'okay' to make the change ? (this has happened in the this
> driver before)
>
> Best regards,
>
> Paulo Almeida
>
>
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies@kernelnewbies.org
> https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
>
No, it isn't OK. If a struct name is changed, it breaks source
compatibility.

-Rogério Valentim

[-- Attachment #1.2: Type: text/html, Size: 4230 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: ioctl number change / backwards compatibility doubt

[Greg KH]

On Sat, Mar 12, 2022 at 01:05:46PM +1300, Paulo Miguel Almeida wrote:
> On Mon, Jan 24, 2022 at 07:20:45AM +0100, Greg KH wrote:
> > On Mon, Jan 24, 2022 at 05:49:06PM +1300, Paulo Miguel Almeida wrote:
> > > On Sun, Jan 23, 2022 at 12:04:48PM +0100, Greg KH wrote:
> > > 
> > > when you told me to look for the userspace tool that interfaced with the
> > > ioctl, my interpretation was that you were referring to something akin
> > > to what /usr/bin/uname utility is to the syscall uname. Please correct me 
> > > if I'm wrong.
> > > 
> > > re: what calls the ioctl created by the driver.
> > > 
> > > I'm led to believe that users of this driver make ioctl sycall
> > > invocations directly from their application's source code like this:
> > > 
> > > #include "pi433_if.h"		/* userspace driver header */
> > > #include <sys/ioctl.h>		/* ioctl */
> > > 
> > > int file_desc = open("/dev/pi433.0", O_RDWR);
> > > struct pi433_tx_cfg tx_cfg = {
> > > 	.frequency = 433000000,
> > > 	.bit_rate = 4800,
> > > 	<omitted for brevity>...
> > > };
> > > 
> > > int ret_val = ioctl(file_desc, PI433_IOC_WR_TX_CFG, tx_cfg);
> > > ....
> > 
> > Yes, sorry for the confusion, this is what I am referring to.  Where is
> > that userspace code as that is the code you will be needing to change if
> > you want to change this ioctl interface.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Hi Greg, 
> 
> The original author replied my email with that question.  He sent me
> some the code used to interface with char device, however, the source
> code he provided me is already incompatible with the current version of
> the driver:
> 
> #include "rfxx.h" (file header name has changed)
> 
> int main(int argc, char *argv[])
> {
> 	...
> 	struct rfxx_send_config sendConfig; (struct name has changed)
> 	...
>         fd = open("/dev/rf69_0.0", O_RDWR); *(char device name changed)*
> 	...
> 	sendConfig.data_mode = packet; *(property doesn't exist)*
>         ...
> 	(IOCTL call has a different name)
> 	ret = ioctl(fd, RFXX_IOC_WR_SEND_CONFIG, &sendConfig);
>         if (ret == -1)
> 	...
> }
> 
> Assuming that I tweak these tools he provided me with and publish them,
> will I then be able to tweak the structures passed back and forth via
> ioctl? (do I need to change ioctl number in this case?) 
> 
> The reason why I'm asking this is because given the fact that other
> developers could have written similar code for interfacing with the
> driver (and that we will never know because code is proprietary), I
> won't be sure that I've changed all occurences at the same time, right?
> 
> All in all, please correct if there are gaps in this train of thought.
> 
> - If a change doesn't break *compiled* code (such as struct renaming) 
> then it's 'okay' to make the change ? (this has happened in the this
> driver before)

For staging drivers, the user/kernel api usually needs to be changed, so
yes, as long as you can change the tools that are being used to talk to
this api, you should be fine.

thanks,

greg k-h

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies