| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <AM9PR04MB86030F36CD0C202F26566B42E7B99@AM9PR04MB8603.eurprd04.prod.outlook.com>
Date: Mon, 13 Mar 2023 14:14:36 +0000
From: Neeraj sanjay kale <neeraj.sanjaykale@....com>
To: Simon Horman <simon.horman@...igine.com>
CC: "davem@...emloft.net" <davem@...emloft.net>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"kuba@...nel.org" <kuba@...nel.org>,
"pabeni@...hat.com" <pabeni@...hat.com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"krzysztof.kozlowski+dt@...aro.org"
<krzysztof.kozlowski+dt@...aro.org>,
"marcel@...tmann.org" <marcel@...tmann.org>,
"johan.hedberg@...il.com" <johan.hedberg@...il.com>,
"luiz.dentz@...il.com" <luiz.dentz@...il.com>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"jirislaby@...nel.org" <jirislaby@...nel.org>,
"alok.a.tiwari@...cle.com" <alok.a.tiwari@...cle.com>,
"hdanton@...a.com" <hdanton@...a.com>,
"ilpo.jarvinen@...ux.intel.com" <ilpo.jarvinen@...ux.intel.com>,
"leon@...nel.org" <leon@...nel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
"linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
Amitkumar Karwar <amitkumar.karwar@....com>,
Rohit Fule <rohit.fule@....com>,
Sherry Sun <sherry.sun@....com>
Subject: Re: [PATCH v8 1/3] serdev: Add method to assert break signal over tty
UART port
Hi Simon,
> > > > > > diff --git a/include/linux/serdev.h b/include/linux/serdev.h
> > > > > > index
> > > > > > 66f624fc618c..c065ef1c82f1 100644
> > > > > > --- a/include/linux/serdev.h
> > > > > > +++ b/include/linux/serdev.h
> > > > >
> > > > > ...
> > > > >
> > > > > > @@ -255,6 +257,10 @@ static inline int
> > > > > > serdev_device_set_tiocm(struct serdev_device *serdev, int set, {
> > > > > > return -ENOTSUPP;
> > > > > > }
> > > > > > +static inline int serdev_device_break_ctl(struct
> > > > > > +serdev_device *serdev, int break_state) {
> > > > > > + return -EOPNOTSUPP;
> > > > >
> > > > > Is the use of -EOPNOTSUPP intentional here?
> > > > > I see -ENOTSUPP is used elsewhere in this file.
> > > > I was suggested to use - EOPNOTSUPP instead of - ENOTSUPP by the
> > > > check
> > > patch scripts and by Leon Romanovsky.
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > >
> patc%2F&data=05%7C01%7Cneeraj.sanjaykale%40nxp.com%7Cd6011072c88
> 94
> > > >
> b141a3008db23bb5bc0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C6
> > > >
> 38143059832335354%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM
> DAiLCJQ
> > > >
> IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata
> =k
> > > >
> gk6w%2Fn2d3rNx%2FXe1rLfN0U%2BeTBJTxztSRUEUW2Noqk%3D&reserved=
> 0
> > > >
> > >
> hwork.kernel.org%2Fproject%2Fbluetooth%2Fpatch%2F20230130180504.202
> > > 944
> > > > 0-2-
> > >
> neeraj.sanjaykale%40nxp.com%2F&data=05%7C01%7Cneeraj.sanjaykale%40
> > > >
> > >
> nxp.com%7Cf2ae2c9ad3c243df2c1a08db232dc0f1%7C686ea1d3bc2b4c6fa92
> > > cd99c5
> > > >
> > >
> c301635%7C0%7C0%7C638142451647332825%7CUnknown%7CTWFpbGZsb3
> > > d8eyJWIjoiM
> > > >
> > >
> C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000
> > > %7C%7C
> > > > %7C&sdata=6cF0gipe4kkwYI6txo0vs8vnmF8azCO6gxQ%2F6Tdyd%2Fw%
> 3D
> > > &reserved=
> > > > 0
> > > >
> > > > ENOTSUPP is not a standard error code and should be avoided in new
> > > patches.
> > > > See:
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > >
> lore%2F&data=05%7C01%7Cneeraj.sanjaykale%40nxp.com%7Cd6011072c88
> 94
> > > >
> b141a3008db23bb5bc0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C6
> > > >
> 38143059832335354%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM
> DAiLCJQ
> > > >
> IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata
> =9
> > > >
> wojVR7aEgzoeajvy%2FMAkSqAYGBkxrJZtyxlvIpeZ%2Bw%3D&reserved=0
> > > > .kernel.org%2Fnetdev%2F20200510182252.GA411829%40lunn.ch%2F&
> data
> > > =05%7C
> > > >
> > >
> 01%7Cneeraj.sanjaykale%40nxp.com%7Cf2ae2c9ad3c243df2c1a08db232dc0f
> > > 1%7C
> > > >
> > >
> 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638142451647332825%
> > > 7CUnknow
> > > >
> > >
> n%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> > > WwiLC
> > > >
> > >
> JXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=wFgYY6VnZ8BBn6Wme8%2BYj
> > > aJRy98qyPnUy
> > > > XC8iCFCv5k%3D&reserved=0
> > >
> > > Thanks.
> > >
> > > I agree that EOPNOTSUPP is preferable.
> > > But my question is if we chose to use it in this case, even if it is
> > > inconsistent with similar code in the same file/API.
> > > If so, then I have no objections.
> >
> > No, it was just to satisfy the check patch error and Leon's comment. The
> driver is happy to check if the serdev returned success or not, and simply
> print the error code during driver debug.
> > Do you think this should be reverted to ENOTSUPP to maintain consistency?
>
> My _opinion_, is that first prize would be converting existing instances of
> ENOTSUPP in this file to EOPNOTSUPP. And then use EOPNOTSUPP going
> forward. And that second prize would be for your patch to use ENOTSUPP.
> Because I think there is a value consistency.
>
> But I do see why you have done things the way you have.
> And I don't necessarily think it is wrong.
When you put it that way, how can I say no to a first prize! :D
I have replaced all instances of ENOTSUPP with EOPNOTSUPP in these 2 files after making sure none of the functions returning it has a check for ENOTSUPP.
It seems most of the instances do not check for a return value anyways.
Please check v9 patch for the changes.
Thanks
Neeraj
Powered by blists - more mailing lists