diff mbox series

[v1] serdev: Add interface serdev_device_ioctl

Message ID 1635753048-5289-1-git-send-email-zijuhu@codeaurora.org (mailing list archive)
State Superseded
Headers show
Series [v1] serdev: Add interface serdev_device_ioctl | expand

Checks

Context Check Description
tedd_an/checkpatch success Checkpatch PASS
tedd_an/gitlint success Gitlint PASS
tedd_an/buildkernel success Build Kernel PASS
tedd_an/testrunnersetup success Test Runner Setup PASS
tedd_an/testrunnerl2cap-tester success Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnerbnep-tester success Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnermgmt-tester fail Total: 468, Passed: 463 (98.9%), Failed: 5, Not Run: 0
tedd_an/testrunnerrfcomm-tester success Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnersco-tester success Total: 12, Passed: 12 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnersmp-tester success Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunneruserchan-tester success Total: 4, Passed: 4 (100.0%), Failed: 0, Not Run: 0

Commit Message

Zijun Hu Nov. 1, 2021, 7:50 a.m. UTC 
From: Zijun Hu <quic_zijuhu@quicinc.com>

For serdev_device which is mounted at virtual tty port, tty ioctl()
maybe be used to make serdev_device ready to talk with tty port, so
add interface serdev_device_ioctl().

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/tty/serdev/core.c           | 11 +++++++++++
 drivers/tty/serdev/serdev-ttyport.c | 12 ++++++++++++
 include/linux/serdev.h              |  9 +++++++++
 3 files changed, 32 insertions(+)

Comments

Greg Kroah-Hartman Nov. 1, 2021, 7:59 a.m. UTC | #1 
On Mon, Nov 01, 2021 at 03:50:48PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> For serdev_device which is mounted at virtual tty port, tty ioctl()
> maybe be used to make serdev_device ready to talk with tty port, so
> add interface serdev_device_ioctl().
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/tty/serdev/core.c           | 11 +++++++++++
>  drivers/tty/serdev/serdev-ttyport.c | 12 ++++++++++++
>  include/linux/serdev.h              |  9 +++++++++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index f1324fe99378..c0f6cd64716b 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -405,6 +405,17 @@ int serdev_device_set_tiocm(struct serdev_device *serdev, int set, int clear)
>  }
>  EXPORT_SYMBOL_GPL(serdev_device_set_tiocm);
>  
> +int serdev_device_ioctl(struct serdev_device *serdev, unsigned int cmd, unsigned long arg)
> +{
> +	struct serdev_controller *ctrl = serdev->ctrl;
> +
> +	if (!ctrl || !ctrl->ops->ioctl)
> +		return -EOPNOTSUPP;

Wrong error for returning that an ioctl is not handled :(

Anyway, what in-tree driver needs this functionality?  Why does serdev
need any ioctl commands?

thanks,

greg k-h
Zijun Hu Nov. 1, 2021, 8:29 a.m. UTC | #2 
On 11/1/2021 3:59 PM, Greg KH wrote:
> On Mon, Nov 01, 2021 at 03:50:48PM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> For serdev_device which is mounted at virtual tty port, tty ioctl()
>> maybe be used to make serdev_device ready to talk with tty port, so
>> add interface serdev_device_ioctl().
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>  drivers/tty/serdev/core.c           | 11 +++++++++++
>>  drivers/tty/serdev/serdev-ttyport.c | 12 ++++++++++++
>>  include/linux/serdev.h              |  9 +++++++++
>>  3 files changed, 32 insertions(+)
>>
>> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
>> index f1324fe99378..c0f6cd64716b 100644
>> --- a/drivers/tty/serdev/core.c
>> +++ b/drivers/tty/serdev/core.c
>> @@ -405,6 +405,17 @@ int serdev_device_set_tiocm(struct serdev_device *serdev, int set, int clear)
>>  }
>>  EXPORT_SYMBOL_GPL(serdev_device_set_tiocm);
>>  
>> +int serdev_device_ioctl(struct serdev_device *serdev, unsigned int cmd, unsigned long arg)
>> +{
>> +	struct serdev_controller *ctrl = serdev->ctrl;
>> +
>> +	if (!ctrl || !ctrl->ops->ioctl)
>> +		return -EOPNOTSUPP;
> 
> Wrong error for returning that an ioctl is not handled :(
checkpatch.pl always reports below WARNING when i use ENOTSUPP as present interfaces
do. so i change error code to EOPNOTSUPP.

#28: FILE: drivers/tty/serdev/core.c:412:
+               return -ENOTSUPP;

WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
> 
> Anyway, what in-tree driver needs this functionality?  Why does serdev
> need any ioctl commands?
>
i am developing driver for a special bluetooth controller which is integrated within SOC,
and it does not connect with the BT HOST with UART as normal controller do, but it has very
similar features as the BT controller with UART I/F. it is mounted on a virtual serial port
driven by a tty driver developed. but it need to call tty ioctl to make the 
special BT controller ready to talk with tty port. so i add this interface.

as you known, the main purpose of ioctl is to achieve MISC and irregular control. so it is useful
for these irregular devices.
 
thanks  
> thanks,
> 
> greg k-h
>
Greg Kroah-Hartman Nov. 1, 2021, 8:32 a.m. UTC | #3 
On Mon, Nov 01, 2021 at 04:29:10PM +0800, Zijun Hu wrote:
> 
> 
> On 11/1/2021 3:59 PM, Greg KH wrote:
> > On Mon, Nov 01, 2021 at 03:50:48PM +0800, Zijun Hu wrote:
> >> From: Zijun Hu <quic_zijuhu@quicinc.com>
> >>
> >> For serdev_device which is mounted at virtual tty port, tty ioctl()
> >> maybe be used to make serdev_device ready to talk with tty port, so
> >> add interface serdev_device_ioctl().
> >>
> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> >> ---
> >>  drivers/tty/serdev/core.c           | 11 +++++++++++
> >>  drivers/tty/serdev/serdev-ttyport.c | 12 ++++++++++++
> >>  include/linux/serdev.h              |  9 +++++++++
> >>  3 files changed, 32 insertions(+)
> >>
> >> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> >> index f1324fe99378..c0f6cd64716b 100644
> >> --- a/drivers/tty/serdev/core.c
> >> +++ b/drivers/tty/serdev/core.c
> >> @@ -405,6 +405,17 @@ int serdev_device_set_tiocm(struct serdev_device *serdev, int set, int clear)
> >>  }
> >>  EXPORT_SYMBOL_GPL(serdev_device_set_tiocm);
> >>  
> >> +int serdev_device_ioctl(struct serdev_device *serdev, unsigned int cmd, unsigned long arg)
> >> +{
> >> +	struct serdev_controller *ctrl = serdev->ctrl;
> >> +
> >> +	if (!ctrl || !ctrl->ops->ioctl)
> >> +		return -EOPNOTSUPP;
> > 
> > Wrong error for returning that an ioctl is not handled :(
> checkpatch.pl always reports below WARNING when i use ENOTSUPP as present interfaces
> do. so i change error code to EOPNOTSUPP.
> 
> #28: FILE: drivers/tty/serdev/core.c:412:
> +               return -ENOTSUPP;
> 
> WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP

Both of them are not the correct error to return when an ioctl is not
supported.

> > Anyway, what in-tree driver needs this functionality?  Why does serdev
> > need any ioctl commands?
> >
> i am developing driver for a special bluetooth controller which is integrated within SOC,
> and it does not connect with the BT HOST with UART as normal controller do, but it has very
> similar features as the BT controller with UART I/F. it is mounted on a virtual serial port
> driven by a tty driver developed. but it need to call tty ioctl to make the 
> special BT controller ready to talk with tty port. so i add this interface.

Please submit this change when you submit your driver that uses it at
the same time so we can review them all at once.  We do not add apis
that are not used in the kernel tree.

> as you known, the main purpose of ioctl is to achieve MISC and irregular control. so it is useful
> for these irregular devices.

For tty devices, "custom" ioctls are not ok, use the standard tty
commands and you should be fine for everything you need to do.

If not, then perhaps your design is incorrect?

thanks,

greg k-h
Zijun Hu Nov. 1, 2021, 9:28 a.m. UTC | #4 
On 11/1/2021 4:32 PM, Greg KH wrote:
> On Mon, Nov 01, 2021 at 04:29:10PM +0800, Zijun Hu wrote:
>>
>>
>> On 11/1/2021 3:59 PM, Greg KH wrote:
>>> On Mon, Nov 01, 2021 at 03:50:48PM +0800, Zijun Hu wrote:
>>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>
>>>> For serdev_device which is mounted at virtual tty port, tty ioctl()
>>>> maybe be used to make serdev_device ready to talk with tty port, so
>>>> add interface serdev_device_ioctl().
>>>>
>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>>> ---
>>>>  drivers/tty/serdev/core.c           | 11 +++++++++++
>>>>  drivers/tty/serdev/serdev-ttyport.c | 12 ++++++++++++
>>>>  include/linux/serdev.h              |  9 +++++++++
>>>>  3 files changed, 32 insertions(+)
>>>>
>>>> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
>>>> index f1324fe99378..c0f6cd64716b 100644
>>>> --- a/drivers/tty/serdev/core.c
>>>> +++ b/drivers/tty/serdev/core.c
>>>> @@ -405,6 +405,17 @@ int serdev_device_set_tiocm(struct serdev_device *serdev, int set, int clear)
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(serdev_device_set_tiocm);
>>>>  
>>>> +int serdev_device_ioctl(struct serdev_device *serdev, unsigned int cmd, unsigned long arg)
>>>> +{
>>>> +	struct serdev_controller *ctrl = serdev->ctrl;
>>>> +
>>>> +	if (!ctrl || !ctrl->ops->ioctl)
>>>> +		return -EOPNOTSUPP;
>>>
>>> Wrong error for returning that an ioctl is not handled :(
>> checkpatch.pl always reports below WARNING when i use ENOTSUPP as present interfaces
>> do. so i change error code to EOPNOTSUPP.
>>
>> #28: FILE: drivers/tty/serdev/core.c:412:
>> +               return -ENOTSUPP;
>>
>> WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
> 
> Both of them are not the correct error to return when an ioctl is not
> supported.
> 
is ENODEV okay?
>>> Anyway, what in-tree driver needs this functionality?  Why does serdev
>>> need any ioctl commands?
>>>
>> i am developing driver for a special bluetooth controller which is integrated within SOC,
>> and it does not connect with the BT HOST with UART as normal controller do, but it has very
>> similar features as the BT controller with UART I/F. it is mounted on a virtual serial port
>> driven by a tty driver developed. but it need to call tty ioctl to make the 
>> special BT controller ready to talk with tty port. so i add this interface.
> 
> Please submit this change when you submit your driver that uses it at
> the same time so we can review them all at once.  We do not add apis
> that are not used in the kernel tree.
> 
okay
>> as you known, the main purpose of ioctl is to achieve MISC and irregular control. so it is useful
>> for these irregular devices.
> 
> For tty devices, "custom" ioctls are not ok, use the standard tty
> commands and you should be fine for everything you need to do.
> 
> If not, then perhaps your design is incorrect?
> 
i just want to refer bt_ioctl within https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/tree/drivers/soc/qcom/bt_tty.c?h=NHSS.QSDK.11.5.0.5.r2
by serdev. so add this interface.
are there any other good solution to advise?
thanks 
> thanks,
> 
> greg k-h
>
Greg Kroah-Hartman Nov. 1, 2021, 11:18 a.m. UTC | #5 
On Mon, Nov 01, 2021 at 05:28:26PM +0800, Zijun Hu wrote:
> 
> 
> On 11/1/2021 4:32 PM, Greg KH wrote:
> > On Mon, Nov 01, 2021 at 04:29:10PM +0800, Zijun Hu wrote:
> >>
> >>
> >> On 11/1/2021 3:59 PM, Greg KH wrote:
> >>> On Mon, Nov 01, 2021 at 03:50:48PM +0800, Zijun Hu wrote:
> >>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
> >>>>
> >>>> For serdev_device which is mounted at virtual tty port, tty ioctl()
> >>>> maybe be used to make serdev_device ready to talk with tty port, so
> >>>> add interface serdev_device_ioctl().
> >>>>
> >>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> >>>> ---
> >>>>  drivers/tty/serdev/core.c           | 11 +++++++++++
> >>>>  drivers/tty/serdev/serdev-ttyport.c | 12 ++++++++++++
> >>>>  include/linux/serdev.h              |  9 +++++++++
> >>>>  3 files changed, 32 insertions(+)
> >>>>
> >>>> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> >>>> index f1324fe99378..c0f6cd64716b 100644
> >>>> --- a/drivers/tty/serdev/core.c
> >>>> +++ b/drivers/tty/serdev/core.c
> >>>> @@ -405,6 +405,17 @@ int serdev_device_set_tiocm(struct serdev_device *serdev, int set, int clear)
> >>>>  }
> >>>>  EXPORT_SYMBOL_GPL(serdev_device_set_tiocm);
> >>>>  
> >>>> +int serdev_device_ioctl(struct serdev_device *serdev, unsigned int cmd, unsigned long arg)
> >>>> +{
> >>>> +	struct serdev_controller *ctrl = serdev->ctrl;
> >>>> +
> >>>> +	if (!ctrl || !ctrl->ops->ioctl)
> >>>> +		return -EOPNOTSUPP;
> >>>
> >>> Wrong error for returning that an ioctl is not handled :(
> >> checkpatch.pl always reports below WARNING when i use ENOTSUPP as present interfaces
> >> do. so i change error code to EOPNOTSUPP.
> >>
> >> #28: FILE: drivers/tty/serdev/core.c:412:
> >> +               return -ENOTSUPP;
> >>
> >> WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
> > 
> > Both of them are not the correct error to return when an ioctl is not
> > supported.
> > 
> is ENODEV okay?

No, -ENOTTY is the correct one as per the documentation, right?

> >>> Anyway, what in-tree driver needs this functionality?  Why does serdev
> >>> need any ioctl commands?
> >>>
> >> i am developing driver for a special bluetooth controller which is integrated within SOC,
> >> and it does not connect with the BT HOST with UART as normal controller do, but it has very
> >> similar features as the BT controller with UART I/F. it is mounted on a virtual serial port
> >> driven by a tty driver developed. but it need to call tty ioctl to make the 
> >> special BT controller ready to talk with tty port. so i add this interface.
> > 
> > Please submit this change when you submit your driver that uses it at
> > the same time so we can review them all at once.  We do not add apis
> > that are not used in the kernel tree.
> > 
> okay
> >> as you known, the main purpose of ioctl is to achieve MISC and irregular control. so it is useful
> >> for these irregular devices.
> > 
> > For tty devices, "custom" ioctls are not ok, use the standard tty
> > commands and you should be fine for everything you need to do.
> > 
> > If not, then perhaps your design is incorrect?
> > 
> i just want to refer bt_ioctl within https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/tree/drivers/soc/qcom/bt_tty.c?h=NHSS.QSDK.11.5.0.5.r2
> by serdev. so add this interface.

The 5.4 kernel is not relevant here, so I do not understand.

> are there any other good solution to advise?

Why not work with the bluetooth developers on this?

thanks,

greg k-h
Marcel Holtmann Nov. 1, 2021, 11:45 a.m. UTC | #6 
Hi Greg,

>>>>>> For serdev_device which is mounted at virtual tty port, tty ioctl()
>>>>>> maybe be used to make serdev_device ready to talk with tty port, so
>>>>>> add interface serdev_device_ioctl().
>>>>>> 
>>>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>>> ---
>>>>>> drivers/tty/serdev/core.c           | 11 +++++++++++
>>>>>> drivers/tty/serdev/serdev-ttyport.c | 12 ++++++++++++
>>>>>> include/linux/serdev.h              |  9 +++++++++
>>>>>> 3 files changed, 32 insertions(+)
>>>>>> 
>>>>>> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
>>>>>> index f1324fe99378..c0f6cd64716b 100644
>>>>>> --- a/drivers/tty/serdev/core.c
>>>>>> +++ b/drivers/tty/serdev/core.c
>>>>>> @@ -405,6 +405,17 @@ int serdev_device_set_tiocm(struct serdev_device *serdev, int set, int clear)
>>>>>> }
>>>>>> EXPORT_SYMBOL_GPL(serdev_device_set_tiocm);
>>>>>> 
>>>>>> +int serdev_device_ioctl(struct serdev_device *serdev, unsigned int cmd, unsigned long arg)
>>>>>> +{
>>>>>> +	struct serdev_controller *ctrl = serdev->ctrl;
>>>>>> +
>>>>>> +	if (!ctrl || !ctrl->ops->ioctl)
>>>>>> +		return -EOPNOTSUPP;
>>>>> 
>>>>> Wrong error for returning that an ioctl is not handled :(
>>>> checkpatch.pl always reports below WARNING when i use ENOTSUPP as present interfaces
>>>> do. so i change error code to EOPNOTSUPP.
>>>> 
>>>> #28: FILE: drivers/tty/serdev/core.c:412:
>>>> +               return -ENOTSUPP;
>>>> 
>>>> WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
>>> 
>>> Both of them are not the correct error to return when an ioctl is not
>>> supported.
>>> 
>> is ENODEV okay?
> 
> No, -ENOTTY is the correct one as per the documentation, right?
> 
>>>>> Anyway, what in-tree driver needs this functionality?  Why does serdev
>>>>> need any ioctl commands?
>>>>> 
>>>> i am developing driver for a special bluetooth controller which is integrated within SOC,
>>>> and it does not connect with the BT HOST with UART as normal controller do, but it has very
>>>> similar features as the BT controller with UART I/F. it is mounted on a virtual serial port
>>>> driven by a tty driver developed. but it need to call tty ioctl to make the 
>>>> special BT controller ready to talk with tty port. so i add this interface.
>>> 
>>> Please submit this change when you submit your driver that uses it at
>>> the same time so we can review them all at once.  We do not add apis
>>> that are not used in the kernel tree.
>>> 
>> okay
>>>> as you known, the main purpose of ioctl is to achieve MISC and irregular control. so it is useful
>>>> for these irregular devices.
>>> 
>>> For tty devices, "custom" ioctls are not ok, use the standard tty
>>> commands and you should be fine for everything you need to do.
>>> 
>>> If not, then perhaps your design is incorrect?
>>> 
>> i just want to refer bt_ioctl within https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/tree/drivers/soc/qcom/bt_tty.c?h=NHSS.QSDK.11.5.0.5.r2
>> by serdev. so add this interface.
> 
> The 5.4 kernel is not relevant here, so I do not understand.
> 
>> are there any other good solution to advise?
> 
> Why not work with the bluetooth developers on this?

if this is just to have some hackish Bluetooth driver, then NAK from my side. Since we have serdev, we have no need for, or requirements for any ioctl anymore. If such thing is needed, it is a bad design.

Regards

Marcel
Greg Kroah-Hartman Nov. 1, 2021, 11:54 a.m. UTC | #7 
On Mon, Nov 01, 2021 at 12:45:36PM +0100, Marcel Holtmann wrote:
> Hi Greg,
> 
> >>>>>> For serdev_device which is mounted at virtual tty port, tty ioctl()
> >>>>>> maybe be used to make serdev_device ready to talk with tty port, so
> >>>>>> add interface serdev_device_ioctl().
> >>>>>> 
> >>>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> >>>>>> ---
> >>>>>> drivers/tty/serdev/core.c           | 11 +++++++++++
> >>>>>> drivers/tty/serdev/serdev-ttyport.c | 12 ++++++++++++
> >>>>>> include/linux/serdev.h              |  9 +++++++++
> >>>>>> 3 files changed, 32 insertions(+)
> >>>>>> 
> >>>>>> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> >>>>>> index f1324fe99378..c0f6cd64716b 100644
> >>>>>> --- a/drivers/tty/serdev/core.c
> >>>>>> +++ b/drivers/tty/serdev/core.c
> >>>>>> @@ -405,6 +405,17 @@ int serdev_device_set_tiocm(struct serdev_device *serdev, int set, int clear)
> >>>>>> }
> >>>>>> EXPORT_SYMBOL_GPL(serdev_device_set_tiocm);
> >>>>>> 
> >>>>>> +int serdev_device_ioctl(struct serdev_device *serdev, unsigned int cmd, unsigned long arg)
> >>>>>> +{
> >>>>>> +	struct serdev_controller *ctrl = serdev->ctrl;
> >>>>>> +
> >>>>>> +	if (!ctrl || !ctrl->ops->ioctl)
> >>>>>> +		return -EOPNOTSUPP;
> >>>>> 
> >>>>> Wrong error for returning that an ioctl is not handled :(
> >>>> checkpatch.pl always reports below WARNING when i use ENOTSUPP as present interfaces
> >>>> do. so i change error code to EOPNOTSUPP.
> >>>> 
> >>>> #28: FILE: drivers/tty/serdev/core.c:412:
> >>>> +               return -ENOTSUPP;
> >>>> 
> >>>> WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
> >>> 
> >>> Both of them are not the correct error to return when an ioctl is not
> >>> supported.
> >>> 
> >> is ENODEV okay?
> > 
> > No, -ENOTTY is the correct one as per the documentation, right?
> > 
> >>>>> Anyway, what in-tree driver needs this functionality?  Why does serdev
> >>>>> need any ioctl commands?
> >>>>> 
> >>>> i am developing driver for a special bluetooth controller which is integrated within SOC,
> >>>> and it does not connect with the BT HOST with UART as normal controller do, but it has very
> >>>> similar features as the BT controller with UART I/F. it is mounted on a virtual serial port
> >>>> driven by a tty driver developed. but it need to call tty ioctl to make the 
> >>>> special BT controller ready to talk with tty port. so i add this interface.
> >>> 
> >>> Please submit this change when you submit your driver that uses it at
> >>> the same time so we can review them all at once.  We do not add apis
> >>> that are not used in the kernel tree.
> >>> 
> >> okay
> >>>> as you known, the main purpose of ioctl is to achieve MISC and irregular control. so it is useful
> >>>> for these irregular devices.
> >>> 
> >>> For tty devices, "custom" ioctls are not ok, use the standard tty
> >>> commands and you should be fine for everything you need to do.
> >>> 
> >>> If not, then perhaps your design is incorrect?
> >>> 
> >> i just want to refer bt_ioctl within https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/tree/drivers/soc/qcom/bt_tty.c?h=NHSS.QSDK.11.5.0.5.r2
> >> by serdev. so add this interface.
> > 
> > The 5.4 kernel is not relevant here, so I do not understand.
> > 
> >> are there any other good solution to advise?
> > 
> > Why not work with the bluetooth developers on this?
> 
> if this is just to have some hackish Bluetooth driver, then NAK from my side. Since we have serdev, we have no need for, or requirements for any ioctl anymore. If such thing is needed, it is a bad design.

Thanks for the confirmation, seems sane to me!

Zijun, please fix up your driver and submit it to be merged and all
should be fine, no need for any custom ioctls.

thanks,

greg k-h
Zijun Hu Nov. 2, 2021, 7:18 a.m. UTC | #8 
On 11/1/2021 7:54 PM, Greg KH wrote:
> On Mon, Nov 01, 2021 at 12:45:36PM +0100, Marcel Holtmann wrote:
>> Hi Greg,
>>
>>>>>>>> For serdev_device which is mounted at virtual tty port, tty ioctl()
>>>>>>>> maybe be used to make serdev_device ready to talk with tty port, so
>>>>>>>> add interface serdev_device_ioctl().
>>>>>>>>
>>>>>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>>>>> ---
>>>>>>>> drivers/tty/serdev/core.c           | 11 +++++++++++
>>>>>>>> drivers/tty/serdev/serdev-ttyport.c | 12 ++++++++++++
>>>>>>>> include/linux/serdev.h              |  9 +++++++++
>>>>>>>> 3 files changed, 32 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
>>>>>>>> index f1324fe99378..c0f6cd64716b 100644
>>>>>>>> --- a/drivers/tty/serdev/core.c
>>>>>>>> +++ b/drivers/tty/serdev/core.c
>>>>>>>> @@ -405,6 +405,17 @@ int serdev_device_set_tiocm(struct serdev_device *serdev, int set, int clear)
>>>>>>>> }
>>>>>>>> EXPORT_SYMBOL_GPL(serdev_device_set_tiocm);
>>>>>>>>
>>>>>>>> +int serdev_device_ioctl(struct serdev_device *serdev, unsigned int cmd, unsigned long arg)
>>>>>>>> +{
>>>>>>>> +	struct serdev_controller *ctrl = serdev->ctrl;
>>>>>>>> +
>>>>>>>> +	if (!ctrl || !ctrl->ops->ioctl)
>>>>>>>> +		return -EOPNOTSUPP;
>>>>>>>
>>>>>>> Wrong error for returning that an ioctl is not handled :(
>>>>>> checkpatch.pl always reports below WARNING when i use ENOTSUPP as present interfaces
>>>>>> do. so i change error code to EOPNOTSUPP.
>>>>>>
>>>>>> #28: FILE: drivers/tty/serdev/core.c:412:
>>>>>> +               return -ENOTSUPP;
>>>>>>
>>>>>> WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
>>>>>
>>>>> Both of them are not the correct error to return when an ioctl is not
>>>>> supported.
>>>>>
>>>> is ENODEV okay?
>>>
>>> No, -ENOTTY is the correct one as per the documentation, right?
>>>
>>>>>>> Anyway, what in-tree driver needs this functionality?  Why does serdev
>>>>>>> need any ioctl commands?
>>>>>>>
>>>>>> i am developing driver for a special bluetooth controller which is integrated within SOC,
>>>>>> and it does not connect with the BT HOST with UART as normal controller do, but it has very
>>>>>> similar features as the BT controller with UART I/F. it is mounted on a virtual serial port
>>>>>> driven by a tty driver developed. but it need to call tty ioctl to make the 
>>>>>> special BT controller ready to talk with tty port. so i add this interface.
>>>>>
>>>>> Please submit this change when you submit your driver that uses it at
>>>>> the same time so we can review them all at once.  We do not add apis
>>>>> that are not used in the kernel tree.
>>>>>
>>>> okay
>>>>>> as you known, the main purpose of ioctl is to achieve MISC and irregular control. so it is useful
>>>>>> for these irregular devices.
>>>>>
>>>>> For tty devices, "custom" ioctls are not ok, use the standard tty
>>>>> commands and you should be fine for everything you need to do.
>>>>>
>>>>> If not, then perhaps your design is incorrect?
>>>>>
>>>> i just want to refer bt_ioctl within https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/tree/drivers/soc/qcom/bt_tty.c?h=NHSS.QSDK.11.5.0.5.r2
>>>> by serdev. so add this interface.
>>>
>>> The 5.4 kernel is not relevant here, so I do not understand.
>>>
>>>> are there any other good solution to advise?
>>>
>>> Why not work with the bluetooth developers on this?
>>
>> if this is just to have some hackish Bluetooth driver, then NAK from my side. Since we have serdev, we have no need for, or requirements for any ioctl anymore. If such thing is needed, it is a bad design.
> 
> Thanks for the confirmation, seems sane to me!
> 
> Zijun, please fix up your driver and submit it to be merged and all
> should be fine, no need for any custom ioctls.
> 
 thank you Greg, i have submitted all changes to support the special BT controller, certainly, it includes this serdev change.
> thanks,
> 
> greg k-h
>
diff mbox series

Patch

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index f1324fe99378..c0f6cd64716b 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -405,6 +405,17 @@  int serdev_device_set_tiocm(struct serdev_device *serdev, int set, int clear)
 }
 EXPORT_SYMBOL_GPL(serdev_device_set_tiocm);
 
+int serdev_device_ioctl(struct serdev_device *serdev, unsigned int cmd, unsigned long arg)
+{
+	struct serdev_controller *ctrl = serdev->ctrl;
+
+	if (!ctrl || !ctrl->ops->ioctl)
+		return -EOPNOTSUPP;
+
+	return ctrl->ops->ioctl(ctrl, cmd, arg);
+}
+EXPORT_SYMBOL_GPL(serdev_device_ioctl);
+
 static int serdev_drv_probe(struct device *dev)
 {
 	const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index d367803e2044..fc6797b26b30 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -247,6 +247,17 @@  static int ttyport_set_tiocm(struct serdev_controller *ctrl, unsigned int set, u
 	return tty->ops->tiocmset(tty, set, clear);
 }
 
+static int ttyport_ioctl(struct serdev_controller *ctrl, unsigned int cmd, unsigned long arg)
+{
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+	struct tty_struct *tty = serport->tty;
+
+	if (!tty->ops->ioctl)
+		return -EOPNOTSUPP;
+
+	return tty->ops->ioctl(tty, cmd, arg);
+}
+
 static const struct serdev_controller_ops ctrl_ops = {
 	.write_buf = ttyport_write_buf,
 	.write_flush = ttyport_write_flush,
@@ -259,6 +270,7 @@  static const struct serdev_controller_ops ctrl_ops = {
 	.wait_until_sent = ttyport_wait_until_sent,
 	.get_tiocm = ttyport_get_tiocm,
 	.set_tiocm = ttyport_set_tiocm,
+	.ioctl = ttyport_ioctl,
 };
 
 struct device *serdev_tty_port_register(struct tty_port *port,
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index 3368c261ab62..3b37bbb187c2 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -91,6 +91,7 @@  struct serdev_controller_ops {
 	void (*wait_until_sent)(struct serdev_controller *, long);
 	int (*get_tiocm)(struct serdev_controller *);
 	int (*set_tiocm)(struct serdev_controller *, unsigned int, unsigned int);
+	int (*ioctl)(struct serdev_controller *ctrl, unsigned int cmd, unsigned long arg);
 };
 
 /**
@@ -201,6 +202,7 @@  int serdev_device_write_buf(struct serdev_device *, const unsigned char *, size_
 void serdev_device_wait_until_sent(struct serdev_device *, long);
 int serdev_device_get_tiocm(struct serdev_device *);
 int serdev_device_set_tiocm(struct serdev_device *, int, int);
+int serdev_device_ioctl(struct serdev_device *serdev, unsigned int cmd, unsigned long arg);
 void serdev_device_write_wakeup(struct serdev_device *);
 int serdev_device_write(struct serdev_device *, const unsigned char *, size_t, long);
 void serdev_device_write_flush(struct serdev_device *);
@@ -254,6 +256,13 @@  static inline int serdev_device_set_tiocm(struct serdev_device *serdev, int set,
 {
 	return -ENOTSUPP;
 }
+
+static inline int serdev_device_ioctl(struct serdev_device *serdev,
+				      unsigned int cmd, unsigned long arg)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int serdev_device_write(struct serdev_device *sdev, const unsigned char *buf,
 				      size_t count, unsigned long timeout)
 {