Re: [PATCH] gigaset: stop maintaining seperately
Thanks to you, Paul, for all your contributions, and specifically for keeping the driver maintained for four more years after I had to abandon it for the same reason. I had a lot of fun working on that driver and I learned a lot in the course. Now it's time to move on without regrets. All the best, Tilman Am 27.07.2019 um 00:05 schrieb Paul Bolle: > The Dutch consumer grade ISDN network will be shut down on September 1, > 2019. This means I'll be converted to some sort of VOIP shortly. At that > point it would be unwise to try to maintain the gigaset driver, even for > odd fixes as I do. So I'll stop maintaining it as a seperate driver and > bump support to CAPI in staging. De facto this means the driver will be > unmaintained, since no-one seems to be working on CAPI. > > I've lighty tested the hardware specific modules of this driver (bas-gigaset, > ser-gigaset, and usb-gigaset) for v5.3-rc1. The basic functionality appears to > be working. It's unclear whether anyone still cares. I'm aware of only one > person sort of using the driver a few years ago. > > Thanks to Karsten Keil for the ISDN subsystems gigaset was using (I4L and > CAPI). And many thanks to Hansjoerg Lipp and Tilman Schmidt for writing and > upstreaming this driver. > > Signed-off-by: Paul Bolle > --- > MAINTAINERS | 7 --- > 1 file changed, 7 deletions(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 783569e3c4b4..e99afbd13355 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -6822,13 +6822,6 @@ F: Documentation/filesystems/gfs2*.txt > F: fs/gfs2/ > F: include/uapi/linux/gfs2_ondisk.h > > -GIGASET ISDN DRIVERS > -M: Paul Bolle > -L: gigaset307x-com...@lists.sourceforge.net > -W: http://gigaset307x.sourceforge.net/ > -S: Odd Fixes > -F: drivers/staging/isdn/gigaset/ > - > GNSS SUBSYSTEM > M: Johan Hovold > T: git git://git.kernel.org/pub/scm/linux/kernel/git/johan/gnss.git >
Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure
On Tue, Sep 27, 2016, at 14:52, SF Markus Elfring wrote: > >> * Is it still correct nowadays that the function "gigaset_initcs" did not > >> call the function "kfree" after a later function call failed? > > > > Yes, if it is handled in another place, Paul already did show you the place. > > To which source code place do you refer here? Obviously the one Paul pointed out to you in detail in his mail dated Mon, 26 Sep 2016 23:13:54 +0200. HTH, Tilman -- Tilman Schmidt til...@imap.cc
Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure
On Tue, Sep 27, 2016, at 14:52, SF Markus Elfring wrote: > >> * Is it still correct nowadays that the function "gigaset_initcs" did not > >> call the function "kfree" after a later function call failed? > > > > Yes, if it is handled in another place, Paul already did show you the place. > > To which source code place do you refer here? Obviously the one Paul pointed out to you in detail in his mail dated Mon, 26 Sep 2016 23:13:54 +0200. HTH, Tilman -- Tilman Schmidt til...@imap.cc
Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure
On Tue, Sep 27, 2016, at 13:32, SF Markus Elfring wrote: > >> I got the impression that the exception handling was incomplete in the > >> implementation of the function "gigaset_initcs". > > > > That impression is wrong. Careful reading of the code will confirm that. > > * Is it still correct nowadays that the function "gigaset_initcs" did not > call the function "kfree" after a later function call failed? Wrong premise. That statement was never correct. > * Do you expect that allocated memory will be automatically reclaimed > after it would return a null pointer? No. Should I? Do you? Regards, Tilman -- Tilman Schmidt til...@imap.cc
Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure
On Tue, Sep 27, 2016, at 13:32, SF Markus Elfring wrote: > >> I got the impression that the exception handling was incomplete in the > >> implementation of the function "gigaset_initcs". > > > > That impression is wrong. Careful reading of the code will confirm that. > > * Is it still correct nowadays that the function "gigaset_initcs" did not > call the function "kfree" after a later function call failed? Wrong premise. That statement was never correct. > * Do you expect that allocated memory will be automatically reclaimed > after it would return a null pointer? No. Should I? Do you? Regards, Tilman -- Tilman Schmidt til...@imap.cc
Re: [PATCH 3/5] ISDN-Gigaset: Delete an error message for a failed memory allocation
On Mon, Sep 26, 2016, at 17:42, SF Markus Elfring wrote: > From: Markus Elfring <elfr...@users.sourceforge.net> > Date: Mon, 26 Sep 2016 15:35:47 +0200 > > Omit an extra message for a memory allocation failure in this function. > > Link: > http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf > > Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net> The patch is fine but the link in the commit message is irrelevant. Please remove it. (Yes, I read through the whole presentation to verify that. It was fun, even.) -- Tilman Schmidt til...@imap.cc
Re: [PATCH 3/5] ISDN-Gigaset: Delete an error message for a failed memory allocation
On Mon, Sep 26, 2016, at 17:42, SF Markus Elfring wrote: > From: Markus Elfring > Date: Mon, 26 Sep 2016 15:35:47 +0200 > > Omit an extra message for a memory allocation failure in this function. > > Link: > http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf > > Signed-off-by: Markus Elfring The patch is fine but the link in the commit message is irrelevant. Please remove it. (Yes, I read through the whole presentation to verify that. It was fun, even.) -- Tilman Schmidt til...@imap.cc
Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure
Hi, as longtime maintainer of the code in question I feel compelled to chime in at this point. On Tue, Sep 27, 2016, at 11:34, SF Markus Elfring wrote: > >> Will it matter here if the function "kfree" will be called for the > >> data structure members "bcs" and "inbuf" after a later function call > >> failed within the implementation of "gigaset_initcs"? > > > > My translation of this question is: could you please hold my hand while > > I read the code of a driver I do not use - a driver for hardware that I > > don't even have, and therefor cannot really test - after I submitted a > > patch that appears to be broken? > > I got the impression that the exception handling was incomplete in the > implementation of the function "gigaset_initcs". That impression is wrong. Careful reading of the code will confirm that. > Does anybody (besides me) care for improving the software situation > there? There's no urgent need for improvement. The code is stable and there's no demonstrated bug to be fixed. You could improve the coding style, but that is of secondary importance, and if you want to do that, as a minimum you have to make sure that you don't introduce new bugs. Thanks, Tilman -- Tilman Schmidt til...@imap.cc
Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure
Hi, as longtime maintainer of the code in question I feel compelled to chime in at this point. On Tue, Sep 27, 2016, at 11:34, SF Markus Elfring wrote: > >> Will it matter here if the function "kfree" will be called for the > >> data structure members "bcs" and "inbuf" after a later function call > >> failed within the implementation of "gigaset_initcs"? > > > > My translation of this question is: could you please hold my hand while > > I read the code of a driver I do not use - a driver for hardware that I > > don't even have, and therefor cannot really test - after I submitted a > > patch that appears to be broken? > > I got the impression that the exception handling was incomplete in the > implementation of the function "gigaset_initcs". That impression is wrong. Careful reading of the code will confirm that. > Does anybody (besides me) care for improving the software situation > there? There's no urgent need for improvement. The code is stable and there's no demonstrated bug to be fixed. You could improve the coding style, but that is of secondary importance, and if you want to do that, as a minimum you have to make sure that you don't introduce new bugs. Thanks, Tilman -- Tilman Schmidt til...@imap.cc
Re: [PATCH 2/2] isdn: i4l: move active-isdn drivers to staging
Am 07.03.2016 um 07:57 schrieb Holger Schurig: > I know that in Germany a good amount of land-line telephone line are > still using ISDN. [...] > Especially company line are using ISDN still, and there are some Linux > programs that act on then, e.g. Asterisk and derived PBX software has > ISDN support which is actively used. AFAIK none of these uses I4L anymore. Asterisk dropped I4L support with version 2 if my memory is correct and nowadays uses CAPI, mISDN or its own DAHDI interface. -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/2] isdn: i4l: move active-isdn drivers to staging
Am 07.03.2016 um 07:57 schrieb Holger Schurig: > I know that in Germany a good amount of land-line telephone line are > still using ISDN. [...] > Especially company line are using ISDN still, and there are some Linux > programs that act on then, e.g. Asterisk and derived PBX software has > ISDN support which is actively used. AFAIK none of these uses I4L anymore. Asterisk dropped I4L support with version 2 if my memory is correct and nowadays uses CAPI, mISDN or its own DAHDI interface. -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/2] isdn: i4l: move active-isdn drivers to staging
Am 10.03.2016 um 17:41 schrieb i...@linux-pingi.de: > Am 10.03.2016 um 13:58 schrieb Paul Bolle: >> On do, 2016-03-10 at 11:53 +0100, i...@linux-pingi.de wrote: >>> mISDN with CAPI support works just fine with pppd and pppdcapiplugin >>> and the CAPI works for all mISDN HW. >> >> In the mainline tree the mISDN and CAPI stacks are effectively separate. Correct. > Since 2012 mISDN has a cAPI20 interface, pure in userspace. To expand: The documented interface for CAPI 2.0 applications is the shared library libcapi20.so. Originally that library just interfaced to the kernel CAPI subsystem through /dev/capi20. Later it was extended to support different access paths to ISDN devices: - via /dev/capi20 and kernel CAPI as before - over the network and a remote CAPI server running rcapid - over the network to FRITZ!Box router via AVM's CAPI-over-TCP service - last but not least, via the mISDNcapid daemon and mISDN Of course this cuts off anything that doesn't pass through libcapi20.so, including applications that (against the standard) access /dev/capi20 directly but also the capidrv.ko i4l compatibility shim. -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/2] isdn: i4l: move active-isdn drivers to staging
Am 10.03.2016 um 17:41 schrieb i...@linux-pingi.de: > Am 10.03.2016 um 13:58 schrieb Paul Bolle: >> On do, 2016-03-10 at 11:53 +0100, i...@linux-pingi.de wrote: >>> mISDN with CAPI support works just fine with pppd and pppdcapiplugin >>> and the CAPI works for all mISDN HW. >> >> In the mainline tree the mISDN and CAPI stacks are effectively separate. Correct. > Since 2012 mISDN has a cAPI20 interface, pure in userspace. To expand: The documented interface for CAPI 2.0 applications is the shared library libcapi20.so. Originally that library just interfaced to the kernel CAPI subsystem through /dev/capi20. Later it was extended to support different access paths to ISDN devices: - via /dev/capi20 and kernel CAPI as before - over the network and a remote CAPI server running rcapid - over the network to FRITZ!Box router via AVM's CAPI-over-TCP service - last but not least, via the mISDNcapid daemon and mISDN Of course this cuts off anything that doesn't pass through libcapi20.so, including applications that (against the standard) access /dev/capi20 directly but also the capidrv.ko i4l compatibility shim. -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/2] isdn: i4l: move active-isdn drivers to staging
Am 07.03.2016 um 09:48 schrieb Paul Bolle: > On za, 2016-03-05 at 14:08 +0100, Tilman Schmidt wrote: >> As a consequence, owners of HiSAX type adapters are in fact stuck with >> the old hisax driver if they want to continue using i4l userspace >> tools. > > Do you know whether or not mISDN tools offer functionality comparable to > i4l tools? AFAICS they don't. mISDN seems very much geared towards voice use, for example with Asterisk. The entire topic of ISDN data connections appears to be missing. I don't see how someone currently using i4l for Internet dial-in (either client or server side) could migrate to mISDN. -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/2] isdn: i4l: move active-isdn drivers to staging
Am 07.03.2016 um 09:48 schrieb Paul Bolle: > On za, 2016-03-05 at 14:08 +0100, Tilman Schmidt wrote: >> As a consequence, owners of HiSAX type adapters are in fact stuck with >> the old hisax driver if they want to continue using i4l userspace >> tools. > > Do you know whether or not mISDN tools offer functionality comparable to > i4l tools? AFAICS they don't. mISDN seems very much geared towards voice use, for example with Asterisk. The entire topic of ISDN data connections appears to be missing. I don't see how someone currently using i4l for Internet dial-in (either client or server side) could migrate to mISDN. -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/2] isdn: i4l: move active-isdn drivers to staging
Am 04.03.2016 um 17:18 schrieb Paul Bolle:
> [Added Tilman and Christoph.]
>
> On vr, 2016-03-04 at 16:24 +0100, Arnd Bergmann wrote:
>> I actually did more patches that I ended up not submitting:
>>
>> * move hisax to staging
>> * remove i4l support from gigaset
>
> For the record: I have no reason to object a patch that does that. (I'm
> not aware anyone complained when gigaset switched its default from i4l
> to capi. By now all relevant distributions should use our capi driver.)
No objection from me either. When the Gigaset driver is built for CAPI
it can still be used from i4l applications via capidrv with no loss of
functionality. That was a primary goal of the CAPI port.
>> * move i4l core to staging
That's a different story. Removing i4l support will actually remove a
userspace visible feature.
> On a local tree I have two (draft) patches that do some related
> preliminary work:
> - isdnhdlc: move into separate directory
> - mISDN: NETJet: stop selecting ISDN_I4L
>
> These trivial patches untangle mISDN and i4l.
That would be a good thing regardless of any decision on the future of
the i4l userspace interface.
> For my part I'm surprised that anyone is still using it. But apparently
> the hardware that required commit 19cebbcb04c8 and 3460baa62068 (which
> I'm unfamiliar with) is still operational. And since there never has
> been, as far as I know, a global i4l to capi migration nor a global i4l
> (and capi) to mISDN migration it might be that some people are stuck on
> i4l drivers for their hardware. Perhaps that explains Cristoph's
> commits.
The trouble is that mISDN never cared about migration or backward
compatibility. So while users of i4l applications have no problem with
i4l drivers being ported to CAPI and dropping native i4l support, they
do have a problem with drivers making that move to mISDN.
That is the situation of the hisax driver today. mISDN started as a
project to migrate hisax to CAPI but regrettably dropped that goal in
favor of a newly invented API leaving old i4l based applications behind.
As a consequence, owners of HiSAX type adapters are in fact stuck with
the old hisax driver if they want to continue using i4l userspace tools.
In my opinion, i4l, capidrv and hisax need to stay in the supported part
for the time being as they are still actively used. Native i4l support
can and should be dropped for hardware with CAPI drivers (ie. gigaset)
but not for hardware with only mISDN drivers (ie. hisax). And finally,
ISDN_CAPI_CAPIDRV should be enabled automatically if both ISDN_I4L and
ISDN_CAPI are enabled, ie. something like:
--- a/drivers/isdn/capi/Kconfig
+++ b/drivers/isdn/capi/Kconfig
@@ -27,8 +27,8 @@ config ISDN_CAPI_MIDDLEWARE
your ISP, say Y here.
config ISDN_CAPI_CAPIDRV
- tristate "CAPI2.0 capidrv interface support"
- depends on ISDN_I4L
+ tristate
+ default ISDN_I4L
help
This option provides the glue code to hook up CAPI driven cards to
the legacy isdn4linux link layer. If you have a card which is
Jm2c,
Tilman
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 2/2] isdn: i4l: move active-isdn drivers to staging
Am 04.03.2016 um 17:18 schrieb Paul Bolle:
> [Added Tilman and Christoph.]
>
> On vr, 2016-03-04 at 16:24 +0100, Arnd Bergmann wrote:
>> I actually did more patches that I ended up not submitting:
>>
>> * move hisax to staging
>> * remove i4l support from gigaset
>
> For the record: I have no reason to object a patch that does that. (I'm
> not aware anyone complained when gigaset switched its default from i4l
> to capi. By now all relevant distributions should use our capi driver.)
No objection from me either. When the Gigaset driver is built for CAPI
it can still be used from i4l applications via capidrv with no loss of
functionality. That was a primary goal of the CAPI port.
>> * move i4l core to staging
That's a different story. Removing i4l support will actually remove a
userspace visible feature.
> On a local tree I have two (draft) patches that do some related
> preliminary work:
> - isdnhdlc: move into separate directory
> - mISDN: NETJet: stop selecting ISDN_I4L
>
> These trivial patches untangle mISDN and i4l.
That would be a good thing regardless of any decision on the future of
the i4l userspace interface.
> For my part I'm surprised that anyone is still using it. But apparently
> the hardware that required commit 19cebbcb04c8 and 3460baa62068 (which
> I'm unfamiliar with) is still operational. And since there never has
> been, as far as I know, a global i4l to capi migration nor a global i4l
> (and capi) to mISDN migration it might be that some people are stuck on
> i4l drivers for their hardware. Perhaps that explains Cristoph's
> commits.
The trouble is that mISDN never cared about migration or backward
compatibility. So while users of i4l applications have no problem with
i4l drivers being ported to CAPI and dropping native i4l support, they
do have a problem with drivers making that move to mISDN.
That is the situation of the hisax driver today. mISDN started as a
project to migrate hisax to CAPI but regrettably dropped that goal in
favor of a newly invented API leaving old i4l based applications behind.
As a consequence, owners of HiSAX type adapters are in fact stuck with
the old hisax driver if they want to continue using i4l userspace tools.
In my opinion, i4l, capidrv and hisax need to stay in the supported part
for the time being as they are still actively used. Native i4l support
can and should be dropped for hardware with CAPI drivers (ie. gigaset)
but not for hardware with only mISDN drivers (ie. hisax). And finally,
ISDN_CAPI_CAPIDRV should be enabled automatically if both ISDN_I4L and
ISDN_CAPI are enabled, ie. something like:
--- a/drivers/isdn/capi/Kconfig
+++ b/drivers/isdn/capi/Kconfig
@@ -27,8 +27,8 @@ config ISDN_CAPI_MIDDLEWARE
your ISP, say Y here.
config ISDN_CAPI_CAPIDRV
- tristate "CAPI2.0 capidrv interface support"
- depends on ISDN_I4L
+ tristate
+ default ISDN_I4L
help
This option provides the glue code to hook up CAPI driven cards to
the legacy isdn4linux link layer. If you have a card which is
Jm2c,
Tilman
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/1] ser_gigaset: use container_of() instead of detour
Am 18.02.2016 um 21:29 schrieb Paul Bolle:
> The purpose of gigaset_device_release() is to kfree() the struct
> ser_cardstate that contains our struct device. This is done via a bit of
> a detour. First we make our struct device's driver_data point to the
> container of our struct ser_cardstate (which is a struct cardstate). In
> gigaset_device_release() we then retrieve that driver_data again. And
> after that we finally kfree() the struct ser_cardstate that was saved in
> the struct cardstate.
>
> All of this can be achieved much easier by using container_of() to get
> from our struct device to its container, struct ser_cardstate. Do so.
You're absolutely right. Very nice!
> Note that at the time the detour was implemented commit b8b2c7d845d5
> ("base/platform: assert that dev_pm_domain callbacks are called
> unconditionally") had just entered the tree. That commit disconnected
> our platform_device and our platform_driver. These were reconnected
> again in v4.5-rc2 through commit 25cad69f21f5 ("base/platform: Fix
> platform drivers with no probe callback"). And one of the consequences
> of that fix was that it broke the detour via driver_data. That's because
> it made __device_release_driver() stop being a NOP for our struct device
> and actually do stuff again. One of the things it now does, is setting
> our driver_data to NULL. That, in turn, makes it impossible for
> gigaset_device_release() to get to our struct cardstate. Which has the
> net effect of leaking a struct ser_cardstate at every call of this
> driver's tty close() operation. So using container_of() has the
> additional benefit of actually working.
>
> Reported-by: Dmitry Vyukov <dvyu...@google.com>
> Tested-by: Dmitry Vyukov <dvyu...@google.com>
> Signed-off-by: Paul Bolle <pebo...@tiscali.nl>
Acked-by: Tilman Schmidt <til...@imap.cc>
Thanks for cleaning up the mess I left behind.
Tilman
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/1] ser_gigaset: use container_of() instead of detour
Am 18.02.2016 um 21:29 schrieb Paul Bolle:
> The purpose of gigaset_device_release() is to kfree() the struct
> ser_cardstate that contains our struct device. This is done via a bit of
> a detour. First we make our struct device's driver_data point to the
> container of our struct ser_cardstate (which is a struct cardstate). In
> gigaset_device_release() we then retrieve that driver_data again. And
> after that we finally kfree() the struct ser_cardstate that was saved in
> the struct cardstate.
>
> All of this can be achieved much easier by using container_of() to get
> from our struct device to its container, struct ser_cardstate. Do so.
You're absolutely right. Very nice!
> Note that at the time the detour was implemented commit b8b2c7d845d5
> ("base/platform: assert that dev_pm_domain callbacks are called
> unconditionally") had just entered the tree. That commit disconnected
> our platform_device and our platform_driver. These were reconnected
> again in v4.5-rc2 through commit 25cad69f21f5 ("base/platform: Fix
> platform drivers with no probe callback"). And one of the consequences
> of that fix was that it broke the detour via driver_data. That's because
> it made __device_release_driver() stop being a NOP for our struct device
> and actually do stuff again. One of the things it now does, is setting
> our driver_data to NULL. That, in turn, makes it impossible for
> gigaset_device_release() to get to our struct cardstate. Which has the
> net effect of leaking a struct ser_cardstate at every call of this
> driver's tty close() operation. So using container_of() has the
> additional benefit of actually working.
>
> Reported-by: Dmitry Vyukov
> Tested-by: Dmitry Vyukov
> Signed-off-by: Paul Bolle
Acked-by: Tilman Schmidt
Thanks for cleaning up the mess I left behind.
Tilman
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
signature.asc
Description: OpenPGP digital signature
Re: [PATCH] gigaset: turn nonsense checks into WARN_ON
Am 09.12.2015 um 21:33 schrieb Alan Cox:
> These checks do nothing useful to protect the code from races. On the other
> hand if the old code has been masking a real bug we would like to know about
> it.
>
> The check for tiocmset is kept because it is valid for a tty driver to have
> a NULL tiocmset method. That in itself is probably a mistake given modern
> coding practices - but needs fixing in the tty layer.
>
> Signed-off-by: Alan Cox
Acked-by: Tilman Schmidt
(Overlooking the nettling subject line.)
> ---
> drivers/isdn/gigaset/ser-gigaset.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/isdn/gigaset/ser-gigaset.c
> b/drivers/isdn/gigaset/ser-gigaset.c
> index d8771b5..8e21f6af 100644
> --- a/drivers/isdn/gigaset/ser-gigaset.c
> +++ b/drivers/isdn/gigaset/ser-gigaset.c
> @@ -67,8 +67,7 @@ static int write_modem(struct cardstate *cs)
> struct sk_buff *skb = bcs->tx_skb;
> int sent = -EOPNOTSUPP;
>
> - if (!tty || !tty->ops || !skb)
> - return -EINVAL;
> + WARN_ON(!tty || !tty->ops || !skb);
>
> if (!skb->len) {
> dev_kfree_skb_any(skb);
> @@ -109,8 +108,7 @@ static int send_cb(struct cardstate *cs)
> unsigned long flags;
> int sent = 0;
>
> - if (!tty || !tty->ops)
> - return -EFAULT;
> + WARN_ON(!tty || !tty->ops);
>
> cb = cs->cmdbuf;
> if (!cb)
> @@ -432,7 +430,9 @@ static int gigaset_set_modem_ctrl(struct cardstate *cs,
> unsigned old_state,
> struct tty_struct *tty = cs->hw.ser->tty;
> unsigned int set, clear;
>
> - if (!tty || !tty->ops || !tty->ops->tiocmset)
> + WARN_ON(!tty || !tty->ops);
> + /* tiocmset is an optional tty driver method */
> + if (!tty->ops->tiocmset)
> return -EINVAL;
> set = new_state & ~old_state;
> clear = old_state & ~new_state;
>
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 0/3] ser_gigaset fixes
Hi Paul, Am 10.12.2015 um 12:31 schrieb Paul Bolle: > 1/3 ran into objections and, I think, Alan Cox is working on an > alternative for it. Would you mind resending 2/3 and 3/3 as a two > patches series? Feel free to add > Acked-by: Paul Bolle > > to both. Alan's patch dated 09 Dec 2015 20:33:24 applies on top of 1/3 so I reckon it should be kept. > (The previous gigaset series, which you sent in July this year, was > picked up from netdev directly by David Miller. Unless people actually > prefer these patches to also be signed-off by me, I'm perfectly fine > with that.) I think as a maintainer you are supposed to sign off patches for the code you maintain. My signed-off as recently retired ex-maintainer was probably still good enough in July. I'm unsure whether it still is today. Regards, Tilman -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure
Hi Peter,
Am 10.12.2015 um 15:04 schrieb Peter Hurley:
>>>> --- a/drivers/isdn/gigaset/ser-gigaset.c
>>>> +++ b/drivers/isdn/gigaset/ser-gigaset.c
>>>> @@ -370,19 +370,23 @@ static void gigaset_freecshw(struct cardstate
>>>> *cs)
>>>>tasklet_kill(>write_tasklet);
>>>>if (!cs->hw.ser)
>>>>return;
>>>> - dev_set_drvdata(>hw.ser->dev.dev, NULL);
>>>>platform_device_unregister(>hw.ser->dev);
>>>> - kfree(cs->hw.ser);
>>>> - cs->hw.ser = NULL;
>>>> }
>>>>
>>>> static void gigaset_device_release(struct device *dev)
>>>> {
>>>>struct platform_device *pdev = to_platform_device(dev);
>>>> + struct cardstate *cs = dev_get_drvdata(dev);
>>>>
>>>>/* adapted from platform_device_release() in
>>>> drivers/base/platform.c */
>>>>kfree(dev->platform_data);
>>>>kfree(pdev->resource);
>>>> +
>>>> + if (!cs)
>>>> + return;
>>>> + dev_set_drvdata(dev, NULL);
>
> This is of marginal value and (I think) unnecessary; it implies
> the core will use the device after release, which would trigger
> many problems if true.
Agreed, but I'm just moving existing code here. Dropping the
dev_set_drvdata() call would be an unrelated change which should be done
in a separate patch if I understand the rules correctly.
Regards,
Tilman
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure
Hi Peter,
Am 10.12.2015 um 15:04 schrieb Peter Hurley:
>>>> --- a/drivers/isdn/gigaset/ser-gigaset.c
>>>> +++ b/drivers/isdn/gigaset/ser-gigaset.c
>>>> @@ -370,19 +370,23 @@ static void gigaset_freecshw(struct cardstate
>>>> *cs)
>>>>tasklet_kill(>write_tasklet);
>>>>if (!cs->hw.ser)
>>>>return;
>>>> - dev_set_drvdata(>hw.ser->dev.dev, NULL);
>>>>platform_device_unregister(>hw.ser->dev);
>>>> - kfree(cs->hw.ser);
>>>> - cs->hw.ser = NULL;
>>>> }
>>>>
>>>> static void gigaset_device_release(struct device *dev)
>>>> {
>>>>struct platform_device *pdev = to_platform_device(dev);
>>>> + struct cardstate *cs = dev_get_drvdata(dev);
>>>>
>>>>/* adapted from platform_device_release() in
>>>> drivers/base/platform.c */
>>>>kfree(dev->platform_data);
>>>>kfree(pdev->resource);
>>>> +
>>>> + if (!cs)
>>>> + return;
>>>> + dev_set_drvdata(dev, NULL);
>
> This is of marginal value and (I think) unnecessary; it implies
> the core will use the device after release, which would trigger
> many problems if true.
Agreed, but I'm just moving existing code here. Dropping the
dev_set_drvdata() call would be an unrelated change which should be done
in a separate patch if I understand the rules correctly.
Regards,
Tilman
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 0/3] ser_gigaset fixes
Hi Paul, Am 10.12.2015 um 12:31 schrieb Paul Bolle: > 1/3 ran into objections and, I think, Alan Cox is working on an > alternative for it. Would you mind resending 2/3 and 3/3 as a two > patches series? Feel free to add > Acked-by: Paul Bolle <pebo...@tiscali.nl> > > to both. Alan's patch dated 09 Dec 2015 20:33:24 applies on top of 1/3 so I reckon it should be kept. > (The previous gigaset series, which you sent in July this year, was > picked up from netdev directly by David Miller. Unless people actually > prefer these patches to also be signed-off by me, I'm perfectly fine > with that.) I think as a maintainer you are supposed to sign off patches for the code you maintain. My signed-off as recently retired ex-maintainer was probably still good enough in July. I'm unsure whether it still is today. Regards, Tilman -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. signature.asc Description: OpenPGP digital signature
Re: [PATCH] gigaset: turn nonsense checks into WARN_ON
Am 09.12.2015 um 21:33 schrieb Alan Cox:
> These checks do nothing useful to protect the code from races. On the other
> hand if the old code has been masking a real bug we would like to know about
> it.
>
> The check for tiocmset is kept because it is valid for a tty driver to have
> a NULL tiocmset method. That in itself is probably a mistake given modern
> coding practices - but needs fixing in the tty layer.
>
> Signed-off-by: Alan Cox <a...@linux.intel.com>
Acked-by: Tilman Schmidt <til...@imap.cc>
(Overlooking the nettling subject line.)
> ---
> drivers/isdn/gigaset/ser-gigaset.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/isdn/gigaset/ser-gigaset.c
> b/drivers/isdn/gigaset/ser-gigaset.c
> index d8771b5..8e21f6af 100644
> --- a/drivers/isdn/gigaset/ser-gigaset.c
> +++ b/drivers/isdn/gigaset/ser-gigaset.c
> @@ -67,8 +67,7 @@ static int write_modem(struct cardstate *cs)
> struct sk_buff *skb = bcs->tx_skb;
> int sent = -EOPNOTSUPP;
>
> - if (!tty || !tty->ops || !skb)
> - return -EINVAL;
> + WARN_ON(!tty || !tty->ops || !skb);
>
> if (!skb->len) {
> dev_kfree_skb_any(skb);
> @@ -109,8 +108,7 @@ static int send_cb(struct cardstate *cs)
> unsigned long flags;
> int sent = 0;
>
> - if (!tty || !tty->ops)
> - return -EFAULT;
> + WARN_ON(!tty || !tty->ops);
>
> cb = cs->cmdbuf;
> if (!cb)
> @@ -432,7 +430,9 @@ static int gigaset_set_modem_ctrl(struct cardstate *cs,
> unsigned old_state,
> struct tty_struct *tty = cs->hw.ser->tty;
> unsigned int set, clear;
>
> - if (!tty || !tty->ops || !tty->ops->tiocmset)
> + WARN_ON(!tty || !tty->ops);
> + /* tiocmset is an optional tty driver method */
> + if (!tty->ops->tiocmset)
> return -EINVAL;
> set = new_state & ~old_state;
> clear = old_state & ~new_state;
>
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure
Am 09.12.2015 um 00:12 schrieb Paul Bolle:
>> --- a/drivers/isdn/gigaset/ser-gigaset.c
>> +++ b/drivers/isdn/gigaset/ser-gigaset.c
>> @@ -370,19 +370,23 @@ static void gigaset_freecshw(struct cardstate
>> *cs)
>> tasklet_kill(>write_tasklet);
>> if (!cs->hw.ser)
>> return;
>> -dev_set_drvdata(>hw.ser->dev.dev, NULL);
>> platform_device_unregister(>hw.ser->dev);
>> -kfree(cs->hw.ser);
>> -cs->hw.ser = NULL;
>> }
>>
>> static void gigaset_device_release(struct device *dev)
>> {
>> struct platform_device *pdev = to_platform_device(dev);
>> +struct cardstate *cs = dev_get_drvdata(dev);
>>
>> /* adapted from platform_device_release() in
>> drivers/base/platform.c */
>> kfree(dev->platform_data);
>> kfree(pdev->resource);
>> +
>> +if (!cs)
>> +return;
>> +dev_set_drvdata(dev, NULL);
>
> dev equals cs->hw.ser->dev.dev, doesn't it?
Correct.
> So what does setting
> cs->hw.ser->dev.dev.driver_data to NULL just before freeing it buy us?
We're freeing cs->hw.ser, not cs->hw.ser->dev.
Clearing the reference to cs from the device structure before freeing cs
guards against possible use-after-free.
>> +kfree(cs->hw.ser);
>> +cs->hw.ser = NULL;
>
> I might be missing something, but what does setting this to NULL buy us
> here?
Just defensive programming. Guarding against possible use-after-free or
double-free.
>
> (I realize that I'm asking questions to code that isn't actually new but
> only moved around, but I think that's still an opportunity to have
> another look at that code.)
I'm a big fan of one change per patch. If we also want to modify the
moved code then that should be done in a separate patch. It makes
bisecting so much easier. Same reason why I separated out patch 3/3. And
btw same reason why I think patch 1/3 should go in as-is, as an obvious
fix to commit f34d7a5b, and any concerns about whether those tests are
useful should be addressed by a separate patch.
Regards,
Tilman
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/3] ser_gigaset: fix up NULL checks
Am 08.12.2015 um 23:16 schrieb One Thousand Gnomes:
> The right fix as far as I can see is to remove the tests although
> WARN_ON() combined with your tty->ops change might be safer.
Feel free to submit a patch.
>> It's pretty obvious that this should have been part of commit
>> f34d7a5b7010 ("tty: The big operations rework"). That being said, these
>
> It ahould probably have been fixed around the same time or in one of the
> tty locking reviews, but drivers/isdn and net/irda weren't traditionally
> part of the general tty maintenance but handled separately/
Or just ignored.
>> test puzzle me. It's not obvious why they're needed. Ie, can the null
>> dereferences they try to catch really happen? But I can try to figure
>> out that in the future, if I ever feel the urge to do so. Anyhow:
>>
>> Acked-by: Paul Bolle
>
> Nacked-by: Alan Cox
So you feel it's better to maintain the current inconsistent state
created by commit f34d7a5b? Please elaborate.
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/3] ser_gigaset: fix up NULL checks
Am 08.12.2015 um 23:16 schrieb One Thousand Gnomes:
> The right fix as far as I can see is to remove the tests although
> WARN_ON() combined with your tty->ops change might be safer.
Feel free to submit a patch.
>> It's pretty obvious that this should have been part of commit
>> f34d7a5b7010 ("tty: The big operations rework"). That being said, these
>
> It ahould probably have been fixed around the same time or in one of the
> tty locking reviews, but drivers/isdn and net/irda weren't traditionally
> part of the general tty maintenance but handled separately/
Or just ignored.
>> test puzzle me. It's not obvious why they're needed. Ie, can the null
>> dereferences they try to catch really happen? But I can try to figure
>> out that in the future, if I ever feel the urge to do so. Anyhow:
>>
>> Acked-by: Paul Bolle
>
> Nacked-by: Alan Cox
So you feel it's better to maintain the current inconsistent state
created by commit f34d7a5b? Please elaborate.
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure
Am 09.12.2015 um 00:12 schrieb Paul Bolle:
>> --- a/drivers/isdn/gigaset/ser-gigaset.c
>> +++ b/drivers/isdn/gigaset/ser-gigaset.c
>> @@ -370,19 +370,23 @@ static void gigaset_freecshw(struct cardstate
>> *cs)
>> tasklet_kill(>write_tasklet);
>> if (!cs->hw.ser)
>> return;
>> -dev_set_drvdata(>hw.ser->dev.dev, NULL);
>> platform_device_unregister(>hw.ser->dev);
>> -kfree(cs->hw.ser);
>> -cs->hw.ser = NULL;
>> }
>>
>> static void gigaset_device_release(struct device *dev)
>> {
>> struct platform_device *pdev = to_platform_device(dev);
>> +struct cardstate *cs = dev_get_drvdata(dev);
>>
>> /* adapted from platform_device_release() in
>> drivers/base/platform.c */
>> kfree(dev->platform_data);
>> kfree(pdev->resource);
>> +
>> +if (!cs)
>> +return;
>> +dev_set_drvdata(dev, NULL);
>
> dev equals cs->hw.ser->dev.dev, doesn't it?
Correct.
> So what does setting
> cs->hw.ser->dev.dev.driver_data to NULL just before freeing it buy us?
We're freeing cs->hw.ser, not cs->hw.ser->dev.
Clearing the reference to cs from the device structure before freeing cs
guards against possible use-after-free.
>> +kfree(cs->hw.ser);
>> +cs->hw.ser = NULL;
>
> I might be missing something, but what does setting this to NULL buy us
> here?
Just defensive programming. Guarding against possible use-after-free or
double-free.
>
> (I realize that I'm asking questions to code that isn't actually new but
> only moved around, but I think that's still an opportunity to have
> another look at that code.)
I'm a big fan of one change per patch. If we also want to modify the
moved code then that should be done in a separate patch. It makes
bisecting so much easier. Same reason why I separated out patch 3/3. And
btw same reason why I think patch 1/3 should go in as-is, as an obvious
fix to commit f34d7a5b, and any concerns about whether those tests are
useful should be addressed by a separate patch.
Regards,
Tilman
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
signature.asc
Description: OpenPGP digital signature
[PATCH 3/3] ser_gigaset: remove unnecessary kfree() calls from release method
device->platform_data and platform_device->resource are never used
and remain NULL through their entire life. Drops the kfree() calls
for them from the device release method.
Signed-off-by: Tilman Schmidt
Reported-by: Paul Bolle
---
drivers/isdn/gigaset/ser-gigaset.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/isdn/gigaset/ser-gigaset.c
b/drivers/isdn/gigaset/ser-gigaset.c
index 2693cb2..b7e0329 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -375,13 +375,8 @@ static void gigaset_freecshw(struct cardstate *cs)
static void gigaset_device_release(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
struct cardstate *cs = dev_get_drvdata(dev);
- /* adapted from platform_device_release() in drivers/base/platform.c */
- kfree(dev->platform_data);
- kfree(pdev->resource);
-
if (!cs)
return;
dev_set_drvdata(dev, NULL);
--
1.9.2.459.g68773ac
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/3] ser_gigaset: fix up NULL checks
Commit f34d7a5b changed tty->driver to tty->ops but left NULL checks
for tty->driver untouched. Fix.
Signed-off-by: Tilman Schmidt
Fixes: f34d7a5b7010 ("tty: The big operations rework")
---
drivers/isdn/gigaset/ser-gigaset.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/isdn/gigaset/ser-gigaset.c
b/drivers/isdn/gigaset/ser-gigaset.c
index 375be50..d8771b5 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -67,7 +67,7 @@ static int write_modem(struct cardstate *cs)
struct sk_buff *skb = bcs->tx_skb;
int sent = -EOPNOTSUPP;
- if (!tty || !tty->driver || !skb)
+ if (!tty || !tty->ops || !skb)
return -EINVAL;
if (!skb->len) {
@@ -109,7 +109,7 @@ static int send_cb(struct cardstate *cs)
unsigned long flags;
int sent = 0;
- if (!tty || !tty->driver)
+ if (!tty || !tty->ops)
return -EFAULT;
cb = cs->cmdbuf;
@@ -432,7 +432,7 @@ static int gigaset_set_modem_ctrl(struct cardstate *cs,
unsigned old_state,
struct tty_struct *tty = cs->hw.ser->tty;
unsigned int set, clear;
- if (!tty || !tty->driver || !tty->ops->tiocmset)
+ if (!tty || !tty->ops || !tty->ops->tiocmset)
return -EINVAL;
set = new_state & ~old_state;
clear = old_state & ~new_state;
--
1.9.2.459.g68773ac
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/3] ser_gigaset: fix deallocation of platform device structure
When shutting down the device, the struct ser_cardstate must not be
kfree()d immediately after the call to platform_device_unregister()
since the embedded struct platform_device is still in use.
Move the kfree() call to the release method instead.
Signed-off-by: Tilman Schmidt
Fixes: 2869b23e4b95 ("drivers/isdn/gigaset: new M101 driver (v2)")
Reported-by: Sasha Levin
---
drivers/isdn/gigaset/ser-gigaset.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/isdn/gigaset/ser-gigaset.c
b/drivers/isdn/gigaset/ser-gigaset.c
index d8771b5..2693cb2 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -370,19 +370,23 @@ static void gigaset_freecshw(struct cardstate *cs)
tasklet_kill(>write_tasklet);
if (!cs->hw.ser)
return;
- dev_set_drvdata(>hw.ser->dev.dev, NULL);
platform_device_unregister(>hw.ser->dev);
- kfree(cs->hw.ser);
- cs->hw.ser = NULL;
}
static void gigaset_device_release(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
+ struct cardstate *cs = dev_get_drvdata(dev);
/* adapted from platform_device_release() in drivers/base/platform.c */
kfree(dev->platform_data);
kfree(pdev->resource);
+
+ if (!cs)
+ return;
+ dev_set_drvdata(dev, NULL);
+ kfree(cs->hw.ser);
+ cs->hw.ser = NULL;
}
/*
--
1.9.2.459.g68773ac
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/3] ser_gigaset fixes
Hi Paul, this series is the result of our discussion on the "freeing an active object" bug. I split my proposed patch into two patches for the separate topics of moving the ser_cardstate kfree() and dropping the useless kfree()s, and also included an unrelated patch (1/3) that had fallen through the cracks in my last series. Patch 2/3 should go into stable releases all the way back to 2.6.32. It applies cleanly to release 3.*/4.* with at most offset 1. For release 2.6.32 there is a trivial merge conflict with a removed comment line. Thanks, Tilman Tilman Schmidt (3): ser_gigaset: fix up NULL checks ser_gigaset: fix deallocation of platform device structure ser_gigaset: remove unnecessary kfree() calls from release method drivers/isdn/gigaset/ser-gigaset.c | 19 +-- 1 file changed, 9 insertions(+), 10 deletions(-) -- 1.9.2.459.g68773ac -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/3] ser_gigaset fixes
Hi Paul, this series is the result of our discussion on the "freeing an active object" bug. I split my proposed patch into two patches for the separate topics of moving the ser_cardstate kfree() and dropping the useless kfree()s, and also included an unrelated patch (1/3) that had fallen through the cracks in my last series. Patch 2/3 should go into stable releases all the way back to 2.6.32. It applies cleanly to release 3.*/4.* with at most offset 1. For release 2.6.32 there is a trivial merge conflict with a removed comment line. Thanks, Tilman Tilman Schmidt (3): ser_gigaset: fix up NULL checks ser_gigaset: fix deallocation of platform device structure ser_gigaset: remove unnecessary kfree() calls from release method drivers/isdn/gigaset/ser-gigaset.c | 19 +-- 1 file changed, 9 insertions(+), 10 deletions(-) -- 1.9.2.459.g68773ac -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/3] ser_gigaset: remove unnecessary kfree() calls from release method
device->platform_data and platform_device->resource are never used
and remain NULL through their entire life. Drops the kfree() calls
for them from the device release method.
Signed-off-by: Tilman Schmidt <til...@imap.cc>
Reported-by: Paul Bolle <pebo...@tiscali.nl>
---
drivers/isdn/gigaset/ser-gigaset.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/isdn/gigaset/ser-gigaset.c
b/drivers/isdn/gigaset/ser-gigaset.c
index 2693cb2..b7e0329 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -375,13 +375,8 @@ static void gigaset_freecshw(struct cardstate *cs)
static void gigaset_device_release(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
struct cardstate *cs = dev_get_drvdata(dev);
- /* adapted from platform_device_release() in drivers/base/platform.c */
- kfree(dev->platform_data);
- kfree(pdev->resource);
-
if (!cs)
return;
dev_set_drvdata(dev, NULL);
--
1.9.2.459.g68773ac
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/3] ser_gigaset: fix up NULL checks
Commit f34d7a5b changed tty->driver to tty->ops but left NULL checks
for tty->driver untouched. Fix.
Signed-off-by: Tilman Schmidt <til...@imap.cc>
Fixes: f34d7a5b7010 ("tty: The big operations rework")
---
drivers/isdn/gigaset/ser-gigaset.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/isdn/gigaset/ser-gigaset.c
b/drivers/isdn/gigaset/ser-gigaset.c
index 375be50..d8771b5 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -67,7 +67,7 @@ static int write_modem(struct cardstate *cs)
struct sk_buff *skb = bcs->tx_skb;
int sent = -EOPNOTSUPP;
- if (!tty || !tty->driver || !skb)
+ if (!tty || !tty->ops || !skb)
return -EINVAL;
if (!skb->len) {
@@ -109,7 +109,7 @@ static int send_cb(struct cardstate *cs)
unsigned long flags;
int sent = 0;
- if (!tty || !tty->driver)
+ if (!tty || !tty->ops)
return -EFAULT;
cb = cs->cmdbuf;
@@ -432,7 +432,7 @@ static int gigaset_set_modem_ctrl(struct cardstate *cs,
unsigned old_state,
struct tty_struct *tty = cs->hw.ser->tty;
unsigned int set, clear;
- if (!tty || !tty->driver || !tty->ops->tiocmset)
+ if (!tty || !tty->ops || !tty->ops->tiocmset)
return -EINVAL;
set = new_state & ~old_state;
clear = old_state & ~new_state;
--
1.9.2.459.g68773ac
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/3] ser_gigaset: fix deallocation of platform device structure
When shutting down the device, the struct ser_cardstate must not be
kfree()d immediately after the call to platform_device_unregister()
since the embedded struct platform_device is still in use.
Move the kfree() call to the release method instead.
Signed-off-by: Tilman Schmidt <til...@imap.cc>
Fixes: 2869b23e4b95 ("drivers/isdn/gigaset: new M101 driver (v2)")
Reported-by: Sasha Levin <sasha.le...@oracle.com>
---
drivers/isdn/gigaset/ser-gigaset.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/isdn/gigaset/ser-gigaset.c
b/drivers/isdn/gigaset/ser-gigaset.c
index d8771b5..2693cb2 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -370,19 +370,23 @@ static void gigaset_freecshw(struct cardstate *cs)
tasklet_kill(>write_tasklet);
if (!cs->hw.ser)
return;
- dev_set_drvdata(>hw.ser->dev.dev, NULL);
platform_device_unregister(>hw.ser->dev);
- kfree(cs->hw.ser);
- cs->hw.ser = NULL;
}
static void gigaset_device_release(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
+ struct cardstate *cs = dev_get_drvdata(dev);
/* adapted from platform_device_release() in drivers/base/platform.c */
kfree(dev->platform_data);
kfree(pdev->resource);
+
+ if (!cs)
+ return;
+ dev_set_drvdata(dev, NULL);
+ kfree(cs->hw.ser);
+ cs->hw.ser = NULL;
}
/*
--
1.9.2.459.g68773ac
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: gigaset: freeing an active object
Am 07.12.2015 um 13:25 schrieb Paul Bolle:
>>> Otherwise we'd still be freeing memory
>>> managed through reference counting.
>>
>> Now I#m confused. I thought by following Peter's suggestion to put the
>> kfree() in the release method we'd avoid just that.
>
> Apparently it does, because I can't trigger the WARNING we're discussing
> here with your patch applied.
Nice.
> I'll have to dive into this stuff again,
> because apparently my mental model of what's going on is incomplete at
> best.
I won't claim anything like completeness for mine.
> In the mean time you might want to turn your patch into something that
> can actually be applied (with or without my Sign-off or Ack; I don't
> care how it finds its way into the tree). Please add add
> Fixes: 2869b23e4b95 ("[PATCH] drivers/isdn/gigaset: new M101 driver (v2)")
Will do. (Not today, though.)
Regards,
Tilman
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
signature.asc
Description: OpenPGP digital signature
Re: gigaset: freeing an active object
Am 06.12.2015 um 21:12 schrieb Paul Bolle:
> On zo, 2015-12-06 at 16:29 +0100, Tilman Schmidt wrote:
>> So the solution might be as simple as moving the kfree() call from
>> gigaset_freecshw() to gigaset_device_release(). Something like this:
>>
>> --- a/drivers/isdn/gigaset/ser-gigaset.c
>> +++ b/drivers/isdn/gigaset/ser-gigaset.c
>> @@ -370,19 +370,18 @@ static void gigaset_freecshw(struct cardstate
>> *cs)
>> tasklet_kill(>write_tasklet);
>> if (!cs->hw.ser)
>> return;
>> - dev_set_drvdata(>hw.ser->dev.dev, NULL);
>> platform_device_unregister(>hw.ser->dev);
>> - kfree(cs->hw.ser);
>> - cs->hw.ser = NULL;
>> }
>>
>> static void gigaset_device_release(struct device *dev)
>> {
>> - struct platform_device *pdev = to_platform_device(dev);
>> + struct cardstate *cs = dev_get_drvdata(dev);
>>
>> - /* adapted from platform_device_release() in drivers/base/platform.c
>> */
>> - kfree(dev->platform_data);
>> - kfree(pdev->resource);
>> + if (!cs)
>> + return;
>> + dev_set_drvdata(dev, NULL);
>> + kfree(cs->hw.ser);
>> + cs->hw.ser = NULL;
>> }
>
> This solution assumes that the struct platform_device is moved out of
> the struct ser_cardstate, doesn't it? In other words, this is something
> to do on top of my (draft) patch.
No, that wasn't my intention. I thought of that solution as an
alternative, not an increment to your patch.
> Otherwise we'd still be freeing memory
> managed through reference counting.
Now I#m confused. I thought by following Peter's suggestion to put the
kfree() in the release method we'd avoid just that.
Regards,
Tilman
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
signature.asc
Description: OpenPGP digital signature
Re: gigaset: freeing an active object
Am 06.12.2015 um 21:12 schrieb Paul Bolle:
> On zo, 2015-12-06 at 16:29 +0100, Tilman Schmidt wrote:
>> So the solution might be as simple as moving the kfree() call from
>> gigaset_freecshw() to gigaset_device_release(). Something like this:
>>
>> --- a/drivers/isdn/gigaset/ser-gigaset.c
>> +++ b/drivers/isdn/gigaset/ser-gigaset.c
>> @@ -370,19 +370,18 @@ static void gigaset_freecshw(struct cardstate
>> *cs)
>> tasklet_kill(>write_tasklet);
>> if (!cs->hw.ser)
>> return;
>> - dev_set_drvdata(>hw.ser->dev.dev, NULL);
>> platform_device_unregister(>hw.ser->dev);
>> - kfree(cs->hw.ser);
>> - cs->hw.ser = NULL;
>> }
>>
>> static void gigaset_device_release(struct device *dev)
>> {
>> - struct platform_device *pdev = to_platform_device(dev);
>> + struct cardstate *cs = dev_get_drvdata(dev);
>>
>> - /* adapted from platform_device_release() in drivers/base/platform.c
>> */
>> - kfree(dev->platform_data);
>> - kfree(pdev->resource);
>> + if (!cs)
>> + return;
>> + dev_set_drvdata(dev, NULL);
>> + kfree(cs->hw.ser);
>> + cs->hw.ser = NULL;
>> }
>
> This solution assumes that the struct platform_device is moved out of
> the struct ser_cardstate, doesn't it? In other words, this is something
> to do on top of my (draft) patch.
No, that wasn't my intention. I thought of that solution as an
alternative, not an increment to your patch.
> Otherwise we'd still be freeing memory
> managed through reference counting.
Now I#m confused. I thought by following Peter's suggestion to put the
kfree() in the release method we'd avoid just that.
Regards,
Tilman
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
signature.asc
Description: OpenPGP digital signature
Re: gigaset: freeing an active object
Am 07.12.2015 um 13:25 schrieb Paul Bolle:
>>> Otherwise we'd still be freeing memory
>>> managed through reference counting.
>>
>> Now I#m confused. I thought by following Peter's suggestion to put the
>> kfree() in the release method we'd avoid just that.
>
> Apparently it does, because I can't trigger the WARNING we're discussing
> here with your patch applied.
Nice.
> I'll have to dive into this stuff again,
> because apparently my mental model of what's going on is incomplete at
> best.
I won't claim anything like completeness for mine.
> In the mean time you might want to turn your patch into something that
> can actually be applied (with or without my Sign-off or Ack; I don't
> care how it finds its way into the tree). Please add add
> Fixes: 2869b23e4b95 ("[PATCH] drivers/isdn/gigaset: new M101 driver (v2)")
Will do. (Not today, though.)
Regards,
Tilman
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
signature.asc
Description: OpenPGP digital signature
Re: gigaset: freeing an active object
Am 06.12.2015 um 14:31 schrieb Paul Bolle:
> On wo, 2015-12-02 at 18:48 -0500, Peter Hurley wrote:
>> On 11/30/2015 01:01 PM, Paul Bolle wrote:
>>> --- a/drivers/isdn/gigaset/ser-gigaset.c
>>> +++ b/drivers/isdn/gigaset/ser-gigaset.c
>>> @@ -42,8 +42,9 @@ MODULE_PARM_DESC(cidmode, "stay in CID mode when
>>> idle");
>>>
>>> static struct gigaset_driver *driver;
>>>
>>> +static struct platform_device pdev;
>>> +
>>> struct ser_cardstate {
>>> - struct platform_device dev;
>>> struct tty_struct *tty;
>>> atomic_trefcnt;
>>> struct completion dead_cmp;
>>> @@ -370,8 +371,8 @@ static void gigaset_freecshw(struct cardstate
>>> *cs)
>>> tasklet_kill(>write_tasklet);
>>> if (!cs->hw.ser)
>>> return;
>>> - dev_set_drvdata(>hw.ser->dev.dev, NULL);
>>> - platform_device_unregister(>hw.ser->dev);
>>> + dev_set_drvdata(, NULL);
>>> + platform_device_unregister();
>>> kfree(cs->hw.ser);
>>
>> Tilman,
>>
>> Is there a 1:1 correspondence and lifetime for the embedded platform
>> device and it's containing memory?
>
> (Haven't heard from Tilman, so I'll give this a try.)
Sorry for that. Been busy.
> That containing memory is a struct ser_cardstate. And currently
> instances of struct _ser_cardstate are malloced and freed in routines
> that also call platform_device_register() and
> platform_device_unregister(). So yes, I think there's a 1:1
> correspondence.
Correct.
>> I ask because the typical approach for device teardown is to put the
>> kfree() in the release method;
>
> (Side note: the (struct device) release method of this driver
> -gigaset_device_release() - is actually a nop. It only frees device
> ->platform_data and platform_device->resource, but neither are actually
> used: they remain NULL through their entire life.)
Yeah, that was just copied unthinkingly from driver/base/platform.c.
So the solution might be as simple as moving the kfree() call from
gigaset_freecshw() to gigaset_device_release(). Something like this:
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -370,19 +370,18 @@ static void gigaset_freecshw(struct cardstate *cs)
tasklet_kill(>write_tasklet);
if (!cs->hw.ser)
return;
- dev_set_drvdata(>hw.ser->dev.dev, NULL);
platform_device_unregister(>hw.ser->dev);
- kfree(cs->hw.ser);
- cs->hw.ser = NULL;
}
static void gigaset_device_release(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
+ struct cardstate *cs = dev_get_drvdata(dev);
- /* adapted from platform_device_release() in
drivers/base/platform.c */
- kfree(dev->platform_data);
- kfree(pdev->resource);
+ if (!cs)
+ return;
+ dev_set_drvdata(dev, NULL);
+ kfree(cs->hw.ser);
+ cs->hw.ser = NULL;
}
/*
(Off the top of my hat, completely untested, don't even know if that
will compile.)
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
signature.asc
Description: OpenPGP digital signature
Re: gigaset: freeing an active object
Am 06.12.2015 um 14:31 schrieb Paul Bolle:
> On wo, 2015-12-02 at 18:48 -0500, Peter Hurley wrote:
>> On 11/30/2015 01:01 PM, Paul Bolle wrote:
>>> --- a/drivers/isdn/gigaset/ser-gigaset.c
>>> +++ b/drivers/isdn/gigaset/ser-gigaset.c
>>> @@ -42,8 +42,9 @@ MODULE_PARM_DESC(cidmode, "stay in CID mode when
>>> idle");
>>>
>>> static struct gigaset_driver *driver;
>>>
>>> +static struct platform_device pdev;
>>> +
>>> struct ser_cardstate {
>>> - struct platform_device dev;
>>> struct tty_struct *tty;
>>> atomic_trefcnt;
>>> struct completion dead_cmp;
>>> @@ -370,8 +371,8 @@ static void gigaset_freecshw(struct cardstate
>>> *cs)
>>> tasklet_kill(>write_tasklet);
>>> if (!cs->hw.ser)
>>> return;
>>> - dev_set_drvdata(>hw.ser->dev.dev, NULL);
>>> - platform_device_unregister(>hw.ser->dev);
>>> + dev_set_drvdata(, NULL);
>>> + platform_device_unregister();
>>> kfree(cs->hw.ser);
>>
>> Tilman,
>>
>> Is there a 1:1 correspondence and lifetime for the embedded platform
>> device and it's containing memory?
>
> (Haven't heard from Tilman, so I'll give this a try.)
Sorry for that. Been busy.
> That containing memory is a struct ser_cardstate. And currently
> instances of struct _ser_cardstate are malloced and freed in routines
> that also call platform_device_register() and
> platform_device_unregister(). So yes, I think there's a 1:1
> correspondence.
Correct.
>> I ask because the typical approach for device teardown is to put the
>> kfree() in the release method;
>
> (Side note: the (struct device) release method of this driver
> -gigaset_device_release() - is actually a nop. It only frees device
> ->platform_data and platform_device->resource, but neither are actually
> used: they remain NULL through their entire life.)
Yeah, that was just copied unthinkingly from driver/base/platform.c.
So the solution might be as simple as moving the kfree() call from
gigaset_freecshw() to gigaset_device_release(). Something like this:
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -370,19 +370,18 @@ static void gigaset_freecshw(struct cardstate *cs)
tasklet_kill(>write_tasklet);
if (!cs->hw.ser)
return;
- dev_set_drvdata(>hw.ser->dev.dev, NULL);
platform_device_unregister(>hw.ser->dev);
- kfree(cs->hw.ser);
- cs->hw.ser = NULL;
}
static void gigaset_device_release(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
+ struct cardstate *cs = dev_get_drvdata(dev);
- /* adapted from platform_device_release() in
drivers/base/platform.c */
- kfree(dev->platform_data);
- kfree(pdev->resource);
+ if (!cs)
+ return;
+ dev_set_drvdata(dev, NULL);
+ kfree(cs->hw.ser);
+ cs->hw.ser = NULL;
}
/*
(Off the top of my hat, completely untested, don't even know if that
will compile.)
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
signature.asc
Description: OpenPGP digital signature
Re: gigaset: freeing an active object
Am 30.11.2015 um 22:07 schrieb Paul Bolle: > How would attaching two devices work with GIGASET_MINORS hardcoded to 1? Ah, it wouldn't. You'd have to recompile with a bigger GIGASET_MINORS. (I wonder if that would actually work. Somehow I still think of GIGASET_MINORS as a configurable value, but it has never been anything but 1 in the entire in-tree history of the driver.) -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. signature.asc Description: OpenPGP digital signature
Re: gigaset: freeing an active object
Am 30.11.2015 um 22:07 schrieb Paul Bolle: > How would attaching two devices work with GIGASET_MINORS hardcoded to 1? Ah, it wouldn't. You'd have to recompile with a bigger GIGASET_MINORS. (I wonder if that would actually work. Somehow I still think of GIGASET_MINORS as a configurable value, but it has never been anything but 1 in the entire in-tree history of the driver.) -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. signature.asc Description: OpenPGP digital signature
Re: gigaset: freeing an active object
Am 30.11.2015 um 19:01 schrieb Paul Bolle: > [DRAFT] gigaset: don't free() a struct platform_device > > One is not supposed to free() a struct platform_device. Instead one > should, in the common case, only call platform_device_unregister(). That > will drop the platform device's reference count. (Actually it's the > reference count of the embedded kobject that is important here. But for > users of platform devices that's basically irrelevant.) > > So move struct platform_device dev out of struct ser_cardstate, because > ser_cardstate is (malloc'ed and) free'd. I wonder how that will behave if someone attaches two of the devices to different serial ports. Not likely, but not forbidden either. Regards, Tilman -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. signature.asc Description: OpenPGP digital signature
Re: gigaset: freeing an active object
Am 30.11.2015 um 19:01 schrieb Paul Bolle: > [DRAFT] gigaset: don't free() a struct platform_device > > One is not supposed to free() a struct platform_device. Instead one > should, in the common case, only call platform_device_unregister(). That > will drop the platform device's reference count. (Actually it's the > reference count of the embedded kobject that is important here. But for > users of platform devices that's basically irrelevant.) > > So move struct platform_device dev out of struct ser_cardstate, because > ser_cardstate is (malloc'ed and) free'd. I wonder how that will behave if someone attaches two of the devices to different serial ports. Not likely, but not forbidden either. Regards, Tilman -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. signature.asc Description: OpenPGP digital signature
Re: gigaset: freeing an active object
Am 29.11.2015 um 19:22 schrieb Peter Hurley: >>> [ 413.536749] WARNING: CPU: 6 PID: 25400 at lib/debugobjects.c:263 >>> debug_print_object+0x1c4/0x1e0() >>> [ 413.538111] ODEBUG: free active (active state 0) object type: timer_list >>> hint: delayed_work_timer_fn+0x0/0x90 >> >> This message seems to indicate that an object of type timer_list was >> freed which was still active. However the driver in question >> (ser_gigaset) does not use any timers. [...] >> Judging from the backtrace below this must be the call >> >> kfree(cs->hw.ser); >> >> in drivers/isdn/gigaset/ser-gigaset.c line 375. >> cs->hw.ser is of type struct ser_cardstate *. >> struct ser_cardstate consists of a struct platform_device, a struct >> completion, an atomic_t and a pointer. No timer_list. [...] > The platform_device embedded in struct ser_cardstate hasn't been released when > you kfree() the memory it's in. Btw I don't see a timer_list object in struct platform_device either. Nor in the embedded struct device. -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. signature.asc Description: OpenPGP digital signature
Re: gigaset: freeing an active object
Am 29.11.2015 um 19:22 schrieb Peter Hurley:
> On 11/29/2015 10:30 AM, Tilman Schmidt wrote:
>>
>> Judging from the backtrace below this must be the call
>>
>> kfree(cs->hw.ser);
>>
>> in drivers/isdn/gigaset/ser-gigaset.c line 375.
>> cs->hw.ser is of type struct ser_cardstate *.
>> struct ser_cardstate consists of a struct platform_device, a struct
>> completion, an atomic_t and a pointer. No timer_list.
[...]
>
> The platform_device embedded in struct ser_cardstate hasn't been released when
> you kfree() the memory it's in.
How so? The kfree() call quoted above is immediately preceded by:
platform_device_unregister(>hw.ser->dev);
Thanks,
Tilman
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
signature.asc
Description: OpenPGP digital signature
Re: gigaset: freeing an active object
Hi Sasha, thanks for the report. As the original author of the code in question, I am somewhat at a loss what to make of it. Am 27.11.2015 um 16:19 schrieb Sasha Levin: > Fuzzing with syzkaller on the latest -next kernel produced this error: Is there a way to know the actual sequence of events that triggered this warning? > [ 413.536749] WARNING: CPU: 6 PID: 25400 at lib/debugobjects.c:263 > debug_print_object+0x1c4/0x1e0() > [ 413.538111] ODEBUG: free active (active state 0) object type: timer_list > hint: delayed_work_timer_fn+0x0/0x90 This message seems to indicate that an object of type timer_list was freed which was still active. However the driver in question (ser_gigaset) does not use any timers. What are the exact conditions for producing this message? IOW how does the ODEBUG code determine that an object of type timer_list is being freed, and that it is still in use? Are there any messages from ser_gigaset or another one of the gigaset drivers before that warning? > [ 413.539598] Modules linked > in:3470693efef57268844f02f5de3ab392d8cf5e209671ddd87163cb964c510659 This message does not tell me anything. What does that hex string after the colon mean? > [ 413.540448] CPU: 6 PID: 25400 Comm: syzkaller_execu Not tainted > 4.4.0-rc2-next-20151126-sasha-5-g00d303e-dirty #2653 > [ 413.547614] Call Trace: > [ 413.548077] [] dump_stack+0x72/0xb7 > [ 413.548765] [] warn_slowpath_common+0x113/0x140 > [ 413.551151] [] warn_slowpath_fmt+0xcb/0x100 > [ 413.554295] [] debug_print_object+0x1c4/0x1e0 > [ 413.556592] [] __debug_check_no_obj_freed+0x215/0x7a0 > [ 413.560526] [] debug_check_no_obj_freed+0x2c/0x40 > [ 413.561328] [] kfree+0x1fc/0x2f0 Judging from the backtrace below this must be the call kfree(cs->hw.ser); in drivers/isdn/gigaset/ser-gigaset.c line 375. cs->hw.ser is of type struct ser_cardstate *. struct ser_cardstate consists of a struct platform_device, a struct completion, an atomic_t and a pointer. No timer_list. > [ 413.561970] [] gigaset_freecshw+0xe1/0x120 There are functions by this name in all three Gigaset hardware dependent modules (bas_gigaset, ser_gigaset and usb_gigaset), but ... > [ 413.562723] [] gigaset_freecs+0x2ad/0x600 > [ 413.564240] [] gigaset_tty_close+0x210/0x280 this function only exists in ser_gigaset. Thanks, Tilman -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. signature.asc Description: OpenPGP digital signature
Re: gigaset: freeing an active object
Am 29.11.2015 um 19:22 schrieb Peter Hurley: >>> [ 413.536749] WARNING: CPU: 6 PID: 25400 at lib/debugobjects.c:263 >>> debug_print_object+0x1c4/0x1e0() >>> [ 413.538111] ODEBUG: free active (active state 0) object type: timer_list >>> hint: delayed_work_timer_fn+0x0/0x90 >> >> This message seems to indicate that an object of type timer_list was >> freed which was still active. However the driver in question >> (ser_gigaset) does not use any timers. [...] >> Judging from the backtrace below this must be the call >> >> kfree(cs->hw.ser); >> >> in drivers/isdn/gigaset/ser-gigaset.c line 375. >> cs->hw.ser is of type struct ser_cardstate *. >> struct ser_cardstate consists of a struct platform_device, a struct >> completion, an atomic_t and a pointer. No timer_list. [...] > The platform_device embedded in struct ser_cardstate hasn't been released when > you kfree() the memory it's in. Btw I don't see a timer_list object in struct platform_device either. Nor in the embedded struct device. -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. signature.asc Description: OpenPGP digital signature
Re: gigaset: freeing an active object
Am 29.11.2015 um 19:22 schrieb Peter Hurley:
> On 11/29/2015 10:30 AM, Tilman Schmidt wrote:
>>
>> Judging from the backtrace below this must be the call
>>
>> kfree(cs->hw.ser);
>>
>> in drivers/isdn/gigaset/ser-gigaset.c line 375.
>> cs->hw.ser is of type struct ser_cardstate *.
>> struct ser_cardstate consists of a struct platform_device, a struct
>> completion, an atomic_t and a pointer. No timer_list.
[...]
>
> The platform_device embedded in struct ser_cardstate hasn't been released when
> you kfree() the memory it's in.
How so? The kfree() call quoted above is immediately preceded by:
platform_device_unregister(>hw.ser->dev);
Thanks,
Tilman
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
signature.asc
Description: OpenPGP digital signature
Re: gigaset: freeing an active object
Hi Sasha, thanks for the report. As the original author of the code in question, I am somewhat at a loss what to make of it. Am 27.11.2015 um 16:19 schrieb Sasha Levin: > Fuzzing with syzkaller on the latest -next kernel produced this error: Is there a way to know the actual sequence of events that triggered this warning? > [ 413.536749] WARNING: CPU: 6 PID: 25400 at lib/debugobjects.c:263 > debug_print_object+0x1c4/0x1e0() > [ 413.538111] ODEBUG: free active (active state 0) object type: timer_list > hint: delayed_work_timer_fn+0x0/0x90 This message seems to indicate that an object of type timer_list was freed which was still active. However the driver in question (ser_gigaset) does not use any timers. What are the exact conditions for producing this message? IOW how does the ODEBUG code determine that an object of type timer_list is being freed, and that it is still in use? Are there any messages from ser_gigaset or another one of the gigaset drivers before that warning? > [ 413.539598] Modules linked > in:3470693efef57268844f02f5de3ab392d8cf5e209671ddd87163cb964c510659 This message does not tell me anything. What does that hex string after the colon mean? > [ 413.540448] CPU: 6 PID: 25400 Comm: syzkaller_execu Not tainted > 4.4.0-rc2-next-20151126-sasha-5-g00d303e-dirty #2653 > [ 413.547614] Call Trace: > [ 413.548077] [] dump_stack+0x72/0xb7 > [ 413.548765] [] warn_slowpath_common+0x113/0x140 > [ 413.551151] [] warn_slowpath_fmt+0xcb/0x100 > [ 413.554295] [] debug_print_object+0x1c4/0x1e0 > [ 413.556592] [] __debug_check_no_obj_freed+0x215/0x7a0 > [ 413.560526] [] debug_check_no_obj_freed+0x2c/0x40 > [ 413.561328] [] kfree+0x1fc/0x2f0 Judging from the backtrace below this must be the call kfree(cs->hw.ser); in drivers/isdn/gigaset/ser-gigaset.c line 375. cs->hw.ser is of type struct ser_cardstate *. struct ser_cardstate consists of a struct platform_device, a struct completion, an atomic_t and a pointer. No timer_list. > [ 413.561970] [] gigaset_freecshw+0xe1/0x120 There are functions by this name in all three Gigaset hardware dependent modules (bas_gigaset, ser_gigaset and usb_gigaset), but ... > [ 413.562723] [] gigaset_freecs+0x2ad/0x600 > [ 413.564240] [] gigaset_tty_close+0x210/0x280 this function only exists in ser_gigaset. Thanks, Tilman -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. signature.asc Description: OpenPGP digital signature
Re: [PATCH 3.12 16/33] isdn/gigaset: reset tty->receive_room when attaching ser_gigaset
Hi Paul, Am 19.10.2015 um 11:09 schrieb Paul Bolle: > On ma, 2015-10-12 at 11:18 +0200, Tilman Schmidt wrote: >> While it doesn't make any sense indeed to run two instances of >> ldattach >> in parallel on one and the same serial port, it is entirely conceivable >> that someone might do so inadvertently, by not being aware that one is >> running already. > > I'm wandering off topic a bit, but doesn't that imply that ldattach > should bail out with an error if someone tries to do that? I'm of two minds about this. On the pro side, it might prevent some surprises. But on the other hand, nothing actually breaks if someone does, and I'm not 100% sure there are really no legitimate scenarios. Add to this that I don't have a clear idea how to actually implement such a bailout, and that I'm really short of time. So I'm reluctant to tackle this topic. Perhaps the best way forward would be someone (not me) submitting a patch to ldattach, thereby triggering a discussion of the pros and cons that would ideally include all (or most) ldattach users and consider all line disciplines it is used with. -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: [PATCH 3.12 16/33] isdn/gigaset: reset tty->receive_room when attaching ser_gigaset
Hi Paul, Am 19.10.2015 um 11:09 schrieb Paul Bolle: > On ma, 2015-10-12 at 11:18 +0200, Tilman Schmidt wrote: >> While it doesn't make any sense indeed to run two instances of >> ldattach >> in parallel on one and the same serial port, it is entirely conceivable >> that someone might do so inadvertently, by not being aware that one is >> running already. > > I'm wandering off topic a bit, but doesn't that imply that ldattach > should bail out with an error if someone tries to do that? I'm of two minds about this. On the pro side, it might prevent some surprises. But on the other hand, nothing actually breaks if someone does, and I'm not 100% sure there are really no legitimate scenarios. Add to this that I don't have a clear idea how to actually implement such a bailout, and that I'm really short of time. So I'm reluctant to tackle this topic. Perhaps the best way forward would be someone (not me) submitting a patch to ldattach, thereby triggering a discussion of the pros and cons that would ideally include all (or most) ldattach users and consider all line disciplines it is used with. -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: [PATCH 3.12 16/33] isdn/gigaset: reset tty->receive_room when attaching ser_gigaset
Paul, Am 06.10.2015 um 23:00 schrieb Paul Bolle: > On ma, 2015-09-21 at 18:07 +0200, Tilman Schmidt wrote: >> Am 21.09.2015 um 15:13 schrieb Peter Hurley: >>> ??? >>> >>> The tool you authored will do it from the command line >>> >>> $ ldattach PPP /dev/ttyS1 >>> $ ldattach GIGASET_M101 /dev/ttyS1 >>> >>> Note that nothing here closes the serial device 'in between', and >>> the tty core has switched directly from PPP to GIGASET_M101. >>> n_tty->receive_room is now 64K. >> >> Indeed it does. I stand corrected. The possibility of running ldattach a >> second time without terminating the first instance didn't occur to me. > > Naive question: when would running ldattach a second time make sense? Peter's argument wasn't about making sense, but about operator error. While it doesn't make any sense indeed to run two instances of ldattach in parallel on one and the same serial port, it is entirely conceivable that someone might do so inadvertently, by not being aware that one is running already. Best Regards, Tilman -- Tilman SchmidtE-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: [PATCH 3.12 16/33] isdn/gigaset: reset tty->receive_room when attaching ser_gigaset
Paul, Am 06.10.2015 um 23:00 schrieb Paul Bolle: > On ma, 2015-09-21 at 18:07 +0200, Tilman Schmidt wrote: >> Am 21.09.2015 um 15:13 schrieb Peter Hurley: >>> ??? >>> >>> The tool you authored will do it from the command line >>> >>> $ ldattach PPP /dev/ttyS1 >>> $ ldattach GIGASET_M101 /dev/ttyS1 >>> >>> Note that nothing here closes the serial device 'in between', and >>> the tty core has switched directly from PPP to GIGASET_M101. >>> n_tty->receive_room is now 64K. >> >> Indeed it does. I stand corrected. The possibility of running ldattach a >> second time without terminating the first instance didn't occur to me. > > Naive question: when would running ldattach a second time make sense? Peter's argument wasn't about making sense, but about operator error. While it doesn't make any sense indeed to run two instances of ldattach in parallel on one and the same serial port, it is entirely conceivable that someone might do so inadvertently, by not being aware that one is running already. Best Regards, Tilman -- Tilman SchmidtE-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
[PATCH v2] Documentation: improve line discipline method descriptions
Mention that the ldisc open method must set tty->receive_room, and that many methods are optional. Add description of receive_buf2 method. Signed-off-by: Tilman Schmidt --- Documentation/serial/tty.txt | 60 1 file changed, 39 insertions(+), 21 deletions(-) diff --git a/Documentation/serial/tty.txt b/Documentation/serial/tty.txt index 973c8ad..bc3842d 100644 --- a/Documentation/serial/tty.txt +++ b/Documentation/serial/tty.txt @@ -39,8 +39,13 @@ TTY side interfaces: open() - Called when the line discipline is attached to the terminal. No other call into the line discipline for this tty will occur until it - completes successfully. Returning an error will - prevent the ldisc from being attached. Can sleep. + completes successfully. Should initialize any + state needed by the ldisc, and set receive_room + in the tty_struct to the maximum amount of data + the line discipline is willing to accept from the + driver with a single call to receive_buf(). + Returning an error will prevent the ldisc from + being attached. Can sleep. close()- This is called on a terminal when the line discipline is being unplugged. At the point of @@ -52,9 +57,16 @@ hangup() - Called when the tty line is hung up. No further calls into the ldisc code will occur. The return value is ignored. Can sleep. -write()- A process is writing data through the line - discipline. Multiple write calls are serialized - by the tty layer for the ldisc. May sleep. +read() - (optional) A process requests reading data from + the line. Multiple read calls may occur in parallel + and the ldisc must deal with serialization issues. + If not defined, the process will receive an EIO + error. May sleep. + +write()- (optional) A process requests writing data to the + line. Multiple write calls are serialized by the + tty layer for the ldisc. If not defined, the + process will receive an EIO error. May sleep. flush_buffer() - (optional) May be called at any point between open and close, and instructs the line discipline @@ -69,27 +81,33 @@ set_termios() - (optional) Called on termios structure changes. termios semaphore so allowed to sleep. Serialized against itself only. -read() - Move data from the line discipline to the user. - Multiple read calls may occur in parallel and the - ldisc must deal with serialization issues. May - sleep. - -poll() - Check the status for the poll/select calls. Multiple - poll calls may occur in parallel. May sleep. +poll() - (optional) Check the status for the poll/select + calls. Multiple poll calls may occur in parallel. + May sleep. -ioctl()- Called when an ioctl is handed to the tty layer - that might be for the ldisc. Multiple ioctl calls - may occur in parallel. May sleep. +ioctl()- (optional) Called when an ioctl is handed to the + tty layer that might be for the ldisc. Multiple + ioctl calls may occur in parallel. May sleep. -compat_ioctl() - Called when a 32 bit ioctl is handed to the tty layer - that might be for the ldisc. Multiple ioctl calls - may occur in parallel. May sleep. +compat_ioctl() - (optional) Called when a 32 bit ioctl is handed + to the tty layer that might be for the ldisc. + Multiple ioctl calls may occur in parallel. + May sleep. Driver Side Interfaces: -receive_buf() - Hand buffers of bytes from the driver to the ldisc - for processing. Semantics currently rather - mysterious 8( +receive_buf() - (optional) Called by the low-level driver to hand + a buffer of received bytes to the ldisc for + processing. The number of bytes is guaranteed not + to exceed the current value of tty->receive_room. + All bytes must be processed. + +receive_buf2() - (optional)
[PATCH v2] Documentation: improve line discipline method descriptions
Mention that the ldisc open method must set tty->receive_room, and that many methods are optional. Add description of receive_buf2 method. Signed-off-by: Tilman Schmidt <til...@imap.cc> --- Documentation/serial/tty.txt | 60 1 file changed, 39 insertions(+), 21 deletions(-) diff --git a/Documentation/serial/tty.txt b/Documentation/serial/tty.txt index 973c8ad..bc3842d 100644 --- a/Documentation/serial/tty.txt +++ b/Documentation/serial/tty.txt @@ -39,8 +39,13 @@ TTY side interfaces: open() - Called when the line discipline is attached to the terminal. No other call into the line discipline for this tty will occur until it - completes successfully. Returning an error will - prevent the ldisc from being attached. Can sleep. + completes successfully. Should initialize any + state needed by the ldisc, and set receive_room + in the tty_struct to the maximum amount of data + the line discipline is willing to accept from the + driver with a single call to receive_buf(). + Returning an error will prevent the ldisc from + being attached. Can sleep. close()- This is called on a terminal when the line discipline is being unplugged. At the point of @@ -52,9 +57,16 @@ hangup() - Called when the tty line is hung up. No further calls into the ldisc code will occur. The return value is ignored. Can sleep. -write()- A process is writing data through the line - discipline. Multiple write calls are serialized - by the tty layer for the ldisc. May sleep. +read() - (optional) A process requests reading data from + the line. Multiple read calls may occur in parallel + and the ldisc must deal with serialization issues. + If not defined, the process will receive an EIO + error. May sleep. + +write()- (optional) A process requests writing data to the + line. Multiple write calls are serialized by the + tty layer for the ldisc. If not defined, the + process will receive an EIO error. May sleep. flush_buffer() - (optional) May be called at any point between open and close, and instructs the line discipline @@ -69,27 +81,33 @@ set_termios() - (optional) Called on termios structure changes. termios semaphore so allowed to sleep. Serialized against itself only. -read() - Move data from the line discipline to the user. - Multiple read calls may occur in parallel and the - ldisc must deal with serialization issues. May - sleep. - -poll() - Check the status for the poll/select calls. Multiple - poll calls may occur in parallel. May sleep. +poll() - (optional) Check the status for the poll/select + calls. Multiple poll calls may occur in parallel. + May sleep. -ioctl()- Called when an ioctl is handed to the tty layer - that might be for the ldisc. Multiple ioctl calls - may occur in parallel. May sleep. +ioctl()- (optional) Called when an ioctl is handed to the + tty layer that might be for the ldisc. Multiple + ioctl calls may occur in parallel. May sleep. -compat_ioctl() - Called when a 32 bit ioctl is handed to the tty layer - that might be for the ldisc. Multiple ioctl calls - may occur in parallel. May sleep. +compat_ioctl() - (optional) Called when a 32 bit ioctl is handed + to the tty layer that might be for the ldisc. + Multiple ioctl calls may occur in parallel. + May sleep. Driver Side Interfaces: -receive_buf() - Hand buffers of bytes from the driver to the ldisc - for processing. Semantics currently rather - mysterious 8( +receive_buf() - (optional) Called by the low-level driver to hand + a buffer of received bytes to the ldisc for + processing. The number of bytes is guaranteed not + to exceed the current value of tty->receive_room. + All bytes must be processed. + +
Re: [PATCH 3.12 16/33] isdn/gigaset: reset tty->receive_room when attaching ser_gigaset
Am 21.09.2015 um 18:54 schrieb Peter Hurley: > On 09/21/2015 09:38 AM, Tilman Schmidt wrote: >> Am 21.09.2015 um 15:13 schrieb Peter Hurley: >>> On 09/18/2015 08:38 AM, Tilman Schmidt wrote: >>>> Am 17.09.2015 um 20:13 schrieb Peter Hurley: >>>>> On Wed, Sep 16, 2015 at 7:26 AM, Tilman Schmidt wrote: >> [...] >>>>>> - The requirement for line disciplines to set receive_room wasn't (and >>>>>> btw still isn't) documented anywhere, so it's unlikely anything actively >>>>>> relied on it. >>>>> >>>>> Nevertheless, that is the requirement, and what every other in-tree line >>>>> discipline does. >>>> >>>> Your word for it. Still I don't understand the curious resistance to >>>> documenting it. If it is the requirement, why keep it secret? >>> >>> Nothing sinister here :) >>> >>> Feel free to submit documentation patches. >> >> I already did. For some unknown reason nobody wants to merge them. > > I vaguely recall that. A quick search reminded me there were unaddressed > comments wrt that patch: https://lkml.org/lkml/2015/7/14/608 Ah, so that's the blocking condition? How can I address that comment in order to unblock that patch? -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: [PATCH 3.12 16/33] isdn/gigaset: reset tty->receive_room when attaching ser_gigaset
Am 21.09.2015 um 15:13 schrieb Peter Hurley: > On 09/18/2015 08:38 AM, Tilman Schmidt wrote: >> Am 17.09.2015 um 20:13 schrieb Peter Hurley: [...] >>> So for example, if you manually set N_PPP (as if by user error) >> >> User error wouldn't suffice, as the LD would get reset to N_TTY when the >> serial device is closed. You would have to write a program that >> deliberately switched the LD first to N_PPP and then to N_GIGASET_M101 >> without closing the device in between. > > ??? > > The tool you authored will do it from the command line > > $ ldattach PPP /dev/ttyS1 > $ ldattach GIGASET_M101 /dev/ttyS1 > > Note that nothing here closes the serial device 'in between', and > the tty core has switched directly from PPP to GIGASET_M101. > n_tty->receive_room is now 64K. Indeed it does. I stand corrected. The possibility of running ldattach a second time without terminating the first instance didn't occur to me. > Please add switching from line disciplines other than N_TTY to your > regression testing. I don't do regression tests for the driver anymore since I stepped down as a maintainer, so that would be up to the present maintainer of ser_gigaset. But I see no reason for that. As I already explained, N_TTY is the only problematic case. >>> and then set this line discipline, tty->receive_room will be 64K, not 4K. >> >> That wouldn't affect the operation of ser_gigaset, > > I've explained this before to you, but here it is again: > > tty->receive_room announces the maximum amt of data the line discipline > can accept from tty core with each call to its receive_buf() method (for > line disciplines that don't provide flow control). > > If the line discipline sets ->receive_room to 64K but can only handle > 8K (as in the case of GIGASET_M101), then data loss should be the expected > result. If you'd care to look at the actual code you'd notice that it truly won't make any difference. The receive_buf() method of ser_gigaset is prepared to drop data and log an error when its receive buffer overflows, no matter how big the block of data passed from tty core is. The only difference a smaller ->receive_room value might possibly make is to distribute the overflowing data to more receive_buf() calls. (Note that the Gigaset M101 device operates at 115200 bits/sec max. so it takes at least 700 msecs to transmit 8k bytes. If we ever get into a situation where tty core actually accumulates more than that amount of data before forwarding it to GIGASET_M101 then we have a more serious problem anyway.) Again, I won't oppose applying this patch to stable releases before 3.10. I just don't see the need, so it would be up to you to advocate such a request. -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: [PATCH 3.12 16/33] isdn/gigaset: reset tty->receive_room when attaching ser_gigaset
Am 21.09.2015 um 15:13 schrieb Peter Hurley: > On 09/18/2015 08:38 AM, Tilman Schmidt wrote: >> Am 17.09.2015 um 20:13 schrieb Peter Hurley: >>> On Wed, Sep 16, 2015 at 7:26 AM, Tilman Schmidt wrote: [...] >>>> - The requirement for line disciplines to set receive_room wasn't (and >>>> btw still isn't) documented anywhere, so it's unlikely anything actively >>>> relied on it. >>> >>> Nevertheless, that is the requirement, and what every other in-tree line >>> discipline does. >> >> Your word for it. Still I don't understand the curious resistance to >> documenting it. If it is the requirement, why keep it secret? > > Nothing sinister here :) > > Feel free to submit documentation patches. I already did. For some unknown reason nobody wants to merge them. -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: [PATCH 3.12 16/33] isdn/gigaset: reset tty->receive_room when attaching ser_gigaset
Am 21.09.2015 um 15:13 schrieb Peter Hurley: > On 09/18/2015 08:38 AM, Tilman Schmidt wrote: >> Am 17.09.2015 um 20:13 schrieb Peter Hurley: >>> On Wed, Sep 16, 2015 at 7:26 AM, Tilman Schmidt <til...@imap.cc> wrote: [...] >>>> - The requirement for line disciplines to set receive_room wasn't (and >>>> btw still isn't) documented anywhere, so it's unlikely anything actively >>>> relied on it. >>> >>> Nevertheless, that is the requirement, and what every other in-tree line >>> discipline does. >> >> Your word for it. Still I don't understand the curious resistance to >> documenting it. If it is the requirement, why keep it secret? > > Nothing sinister here :) > > Feel free to submit documentation patches. I already did. For some unknown reason nobody wants to merge them. -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: [PATCH 3.12 16/33] isdn/gigaset: reset tty->receive_room when attaching ser_gigaset
Am 21.09.2015 um 15:13 schrieb Peter Hurley: > On 09/18/2015 08:38 AM, Tilman Schmidt wrote: >> Am 17.09.2015 um 20:13 schrieb Peter Hurley: [...] >>> So for example, if you manually set N_PPP (as if by user error) >> >> User error wouldn't suffice, as the LD would get reset to N_TTY when the >> serial device is closed. You would have to write a program that >> deliberately switched the LD first to N_PPP and then to N_GIGASET_M101 >> without closing the device in between. > > ??? > > The tool you authored will do it from the command line > > $ ldattach PPP /dev/ttyS1 > $ ldattach GIGASET_M101 /dev/ttyS1 > > Note that nothing here closes the serial device 'in between', and > the tty core has switched directly from PPP to GIGASET_M101. > n_tty->receive_room is now 64K. Indeed it does. I stand corrected. The possibility of running ldattach a second time without terminating the first instance didn't occur to me. > Please add switching from line disciplines other than N_TTY to your > regression testing. I don't do regression tests for the driver anymore since I stepped down as a maintainer, so that would be up to the present maintainer of ser_gigaset. But I see no reason for that. As I already explained, N_TTY is the only problematic case. >>> and then set this line discipline, tty->receive_room will be 64K, not 4K. >> >> That wouldn't affect the operation of ser_gigaset, > > I've explained this before to you, but here it is again: > > tty->receive_room announces the maximum amt of data the line discipline > can accept from tty core with each call to its receive_buf() method (for > line disciplines that don't provide flow control). > > If the line discipline sets ->receive_room to 64K but can only handle > 8K (as in the case of GIGASET_M101), then data loss should be the expected > result. If you'd care to look at the actual code you'd notice that it truly won't make any difference. The receive_buf() method of ser_gigaset is prepared to drop data and log an error when its receive buffer overflows, no matter how big the block of data passed from tty core is. The only difference a smaller ->receive_room value might possibly make is to distribute the overflowing data to more receive_buf() calls. (Note that the Gigaset M101 device operates at 115200 bits/sec max. so it takes at least 700 msecs to transmit 8k bytes. If we ever get into a situation where tty core actually accumulates more than that amount of data before forwarding it to GIGASET_M101 then we have a more serious problem anyway.) Again, I won't oppose applying this patch to stable releases before 3.10. I just don't see the need, so it would be up to you to advocate such a request. -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: [PATCH 3.12 16/33] isdn/gigaset: reset tty->receive_room when attaching ser_gigaset
Am 21.09.2015 um 18:54 schrieb Peter Hurley: > On 09/21/2015 09:38 AM, Tilman Schmidt wrote: >> Am 21.09.2015 um 15:13 schrieb Peter Hurley: >>> On 09/18/2015 08:38 AM, Tilman Schmidt wrote: >>>> Am 17.09.2015 um 20:13 schrieb Peter Hurley: >>>>> On Wed, Sep 16, 2015 at 7:26 AM, Tilman Schmidt <til...@imap.cc> wrote: >> [...] >>>>>> - The requirement for line disciplines to set receive_room wasn't (and >>>>>> btw still isn't) documented anywhere, so it's unlikely anything actively >>>>>> relied on it. >>>>> >>>>> Nevertheless, that is the requirement, and what every other in-tree line >>>>> discipline does. >>>> >>>> Your word for it. Still I don't understand the curious resistance to >>>> documenting it. If it is the requirement, why keep it secret? >>> >>> Nothing sinister here :) >>> >>> Feel free to submit documentation patches. >> >> I already did. For some unknown reason nobody wants to merge them. > > I vaguely recall that. A quick search reminded me there were unaddressed > comments wrt that patch: https://lkml.org/lkml/2015/7/14/608 Ah, so that's the blocking condition? How can I address that comment in order to unblock that patch? -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: [PATCH 3.12 16/33] isdn/gigaset: reset tty->receive_room when attaching ser_gigaset
Am 17.09.2015 um 20:13 schrieb Peter Hurley:
> On Wed, Sep 16, 2015 at 7:26 AM, Tilman Schmidt wrote:
>> Am 16.09.2015 um 03:18 schrieb Peter Hurley:
>>> On Tue, Sep 15, 2015 at 8:37 PM, Tilman Schmidt wrote:
>>>> Am 16.09.2015 um 01:08 schrieb Peter Hurley:
>>>>> On Tue, Sep 15, 2015 at 10:22 AM, Jiri Slaby wrote:
>>>>>
>>>>> From: Tilman Schmidt
>>>>>
>>>>> 3.12-stable review patch. If anyone has any objections, please let
>>>>> me know.
>>>>>
>>>>> ===
>>>>>
>>>>> [ Upstream commit fd98e9419d8d622a4de91f76b306af6aa627aa9c ]
>>>>>
>>>>> Commit 79901317ce80 ("n_tty: Don't flush buffer when closing ldisc"),
>>>>> first merged in kernel release 3.10, caused the following regression
>>>>> in the Gigaset M101 driver:
>>>>>
>>>>>
>>>>> Again, I'll just note my objection to this commit log.
>>>>>
>>>>> This driver was always broken because it never initialized
>>>>> tty->receive_room,
>>>>> but rather relied on common but not guaranteed circumstances to
>>>>> function.
>>>>>
>>>>> The commit noted simply made the underlying bug more evident, but the
>>>>> root cause was from the original merge commit of this driver.
>>>>
>>>> I must admit I still don't understand that objection. The meaning of the
>>>> term "regression" is simply that something which previously worked
>>>> stopped working. It doesn't imply any statement about the root cause.
>>>>
>>>> The ser-gigaset driver worked before the introduction of commit
>>>> 79901317ce80. It didn't work anymore after the introduction of that
>>>> commit. So it is correct, and does not contradict your statements above
>>>> in any way, to state that commit introduced the described regression.
>>>
>>> By asserting that commit 79901317ce80 caused the regression, you're
>>> claiming that this fix is unnecessary for kernel versions prior to 3.10
>>
>> Correct.
>>
>>> Are you certain that no other sequence of state leads to the same
>>> condition (and thus requiring the same fix) in earlier kernel versions?
>>
>> Reasonably certain, yes, for three reasons:
>> - There where no reports of that problem before 3.10.
>
>
>
>> - My own tests did never encounter that condition, and even after being
>> made aware of it I was not able to come up with a test that would
>> provoke it with a kernel version before 3.10.
>
> Do any of your tests switch to this line discipline from any other than N_TTY?
Of course not. That wouldn't make any sense.
> So for example, if you manually set N_PPP (as if by user error)
User error wouldn't suffice, as the LD would get reset to N_TTY when the
serial device is closed. You would have to write a program that
deliberately switched the LD first to N_PPP and then to N_GIGASET_M101
without closing the device in between.
> and then set this line discipline, tty->receive_room will be 64K, not 4K.
That wouldn't affect the operation of ser_gigaset, so even if I had set
up such a contrived test scenario it wouldn't have exposed any problem.
Only setting tty->receive_room to 0 causes the problem, and N_TTY with
commit 79901317ce80 is the only LD which does that.
>> - The requirement for line disciplines to set receive_room wasn't (and
>> btw still isn't) documented anywhere, so it's unlikely anything actively
>> relied on it.
>
> Nevertheless, that is the requirement, and what every other in-tree line
> discipline does.
Your word for it. Still I don't understand the curious resistance to
documenting it. If it is the requirement, why keep it secret?
Regards,
Tilman
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 3.12 16/33] isdn/gigaset: reset tty->receive_room when attaching ser_gigaset
Am 17.09.2015 um 20:13 schrieb Peter Hurley:
> On Wed, Sep 16, 2015 at 7:26 AM, Tilman Schmidt <til...@imap.cc> wrote:
>> Am 16.09.2015 um 03:18 schrieb Peter Hurley:
>>> On Tue, Sep 15, 2015 at 8:37 PM, Tilman Schmidt <til...@imap.cc> wrote:
>>>> Am 16.09.2015 um 01:08 schrieb Peter Hurley:
>>>>> On Tue, Sep 15, 2015 at 10:22 AM, Jiri Slaby <jsl...@suse.cz> wrote:
>>>>>
>>>>> From: Tilman Schmidt <til...@imap.cc>
>>>>>
>>>>> 3.12-stable review patch. If anyone has any objections, please let
>>>>> me know.
>>>>>
>>>>> ===
>>>>>
>>>>> [ Upstream commit fd98e9419d8d622a4de91f76b306af6aa627aa9c ]
>>>>>
>>>>> Commit 79901317ce80 ("n_tty: Don't flush buffer when closing ldisc"),
>>>>> first merged in kernel release 3.10, caused the following regression
>>>>> in the Gigaset M101 driver:
>>>>>
>>>>>
>>>>> Again, I'll just note my objection to this commit log.
>>>>>
>>>>> This driver was always broken because it never initialized
>>>>> tty->receive_room,
>>>>> but rather relied on common but not guaranteed circumstances to
>>>>> function.
>>>>>
>>>>> The commit noted simply made the underlying bug more evident, but the
>>>>> root cause was from the original merge commit of this driver.
>>>>
>>>> I must admit I still don't understand that objection. The meaning of the
>>>> term "regression" is simply that something which previously worked
>>>> stopped working. It doesn't imply any statement about the root cause.
>>>>
>>>> The ser-gigaset driver worked before the introduction of commit
>>>> 79901317ce80. It didn't work anymore after the introduction of that
>>>> commit. So it is correct, and does not contradict your statements above
>>>> in any way, to state that commit introduced the described regression.
>>>
>>> By asserting that commit 79901317ce80 caused the regression, you're
>>> claiming that this fix is unnecessary for kernel versions prior to 3.10
>>
>> Correct.
>>
>>> Are you certain that no other sequence of state leads to the same
>>> condition (and thus requiring the same fix) in earlier kernel versions?
>>
>> Reasonably certain, yes, for three reasons:
>> - There where no reports of that problem before 3.10.
>
>
>
>> - My own tests did never encounter that condition, and even after being
>> made aware of it I was not able to come up with a test that would
>> provoke it with a kernel version before 3.10.
>
> Do any of your tests switch to this line discipline from any other than N_TTY?
Of course not. That wouldn't make any sense.
> So for example, if you manually set N_PPP (as if by user error)
User error wouldn't suffice, as the LD would get reset to N_TTY when the
serial device is closed. You would have to write a program that
deliberately switched the LD first to N_PPP and then to N_GIGASET_M101
without closing the device in between.
> and then set this line discipline, tty->receive_room will be 64K, not 4K.
That wouldn't affect the operation of ser_gigaset, so even if I had set
up such a contrived test scenario it wouldn't have exposed any problem.
Only setting tty->receive_room to 0 causes the problem, and N_TTY with
commit 79901317ce80 is the only LD which does that.
>> - The requirement for line disciplines to set receive_room wasn't (and
>> btw still isn't) documented anywhere, so it's unlikely anything actively
>> relied on it.
>
> Nevertheless, that is the requirement, and what every other in-tree line
> discipline does.
Your word for it. Still I don't understand the curious resistance to
documenting it. If it is the requirement, why keep it secret?
Regards,
Tilman
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 3.12 16/33] isdn/gigaset: reset tty->receive_room when attaching ser_gigaset
Am 16.09.2015 um 03:18 schrieb Peter Hurley:
> On Tue, Sep 15, 2015 at 8:37 PM, Tilman Schmidt wrote:
>> Am 16.09.2015 um 01:08 schrieb Peter Hurley:
>>> On Tue, Sep 15, 2015 at 10:22 AM, Jiri Slaby wrote:
>>>
>>> From: Tilman Schmidt
>>>
>>> 3.12-stable review patch. If anyone has any objections, please let
>>> me know.
>>>
>>> ===
>>>
>>> [ Upstream commit fd98e9419d8d622a4de91f76b306af6aa627aa9c ]
>>>
>>> Commit 79901317ce80 ("n_tty: Don't flush buffer when closing ldisc"),
>>> first merged in kernel release 3.10, caused the following regression
>>> in the Gigaset M101 driver:
>>>
>>>
>>> Again, I'll just note my objection to this commit log.
>>>
>>> This driver was always broken because it never initialized
>>> tty->receive_room,
>>> but rather relied on common but not guaranteed circumstances to
>>> function.
>>>
>>> The commit noted simply made the underlying bug more evident, but the
>>> root cause was from the original merge commit of this driver.
>>
>> I must admit I still don't understand that objection. The meaning of the
>> term "regression" is simply that something which previously worked
>> stopped working. It doesn't imply any statement about the root cause.
>>
>> The ser-gigaset driver worked before the introduction of commit
>> 79901317ce80. It didn't work anymore after the introduction of that
>> commit. So it is correct, and does not contradict your statements above
>> in any way, to state that commit introduced the described regression.
>
> By asserting that commit 79901317ce80 caused the regression, you're
> claiming that this fix is unnecessary for kernel versions prior to 3.10
Correct.
> Are you certain that no other sequence of state leads to the same
> condition (and thus requiring the same fix) in earlier kernel versions?
Reasonably certain, yes, for three reasons:
- There where no reports of that problem before 3.10.
- My own tests did never encounter that condition, and even after being
made aware of it I was not able to come up with a test that would
provoke it with a kernel version before 3.10.
- The requirement for line disciplines to set receive_room wasn't (and
btw still isn't) documented anywhere, so it's unlikely anything actively
relied on it.
But if you want to propose that patch for inclusion in earlier stable
releases I won't oppose it.
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 3.12 16/33] isdn/gigaset: reset tty->receive_room when attaching ser_gigaset
Am 16.09.2015 um 03:18 schrieb Peter Hurley:
> On Tue, Sep 15, 2015 at 8:37 PM, Tilman Schmidt <til...@imap.cc> wrote:
>> Am 16.09.2015 um 01:08 schrieb Peter Hurley:
>>> On Tue, Sep 15, 2015 at 10:22 AM, Jiri Slaby <jsl...@suse.cz> wrote:
>>>
>>> From: Tilman Schmidt <til...@imap.cc>
>>>
>>> 3.12-stable review patch. If anyone has any objections, please let
>>> me know.
>>>
>>> ===
>>>
>>> [ Upstream commit fd98e9419d8d622a4de91f76b306af6aa627aa9c ]
>>>
>>> Commit 79901317ce80 ("n_tty: Don't flush buffer when closing ldisc"),
>>> first merged in kernel release 3.10, caused the following regression
>>> in the Gigaset M101 driver:
>>>
>>>
>>> Again, I'll just note my objection to this commit log.
>>>
>>> This driver was always broken because it never initialized
>>> tty->receive_room,
>>> but rather relied on common but not guaranteed circumstances to
>>> function.
>>>
>>> The commit noted simply made the underlying bug more evident, but the
>>> root cause was from the original merge commit of this driver.
>>
>> I must admit I still don't understand that objection. The meaning of the
>> term "regression" is simply that something which previously worked
>> stopped working. It doesn't imply any statement about the root cause.
>>
>> The ser-gigaset driver worked before the introduction of commit
>> 79901317ce80. It didn't work anymore after the introduction of that
>> commit. So it is correct, and does not contradict your statements above
>> in any way, to state that commit introduced the described regression.
>
> By asserting that commit 79901317ce80 caused the regression, you're
> claiming that this fix is unnecessary for kernel versions prior to 3.10
Correct.
> Are you certain that no other sequence of state leads to the same
> condition (and thus requiring the same fix) in earlier kernel versions?
Reasonably certain, yes, for three reasons:
- There where no reports of that problem before 3.10.
- My own tests did never encounter that condition, and even after being
made aware of it I was not able to come up with a test that would
provoke it with a kernel version before 3.10.
- The requirement for line disciplines to set receive_room wasn't (and
btw still isn't) documented anywhere, so it's unlikely anything actively
relied on it.
But if you want to propose that patch for inclusion in earlier stable
releases I won't oppose it.
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 3.12 16/33] isdn/gigaset: reset tty->receive_room when attaching ser_gigaset
Am 16.09.2015 um 01:08 schrieb Peter Hurley:
> On Tue, Sep 15, 2015 at 10:22 AM, Jiri Slaby <mailto:jsl...@suse.cz>> wrote:
>
> From: Tilman Schmidt
>
> 3.12-stable review patch. If anyone has any objections, please let
> me know.
>
> ===
>
> [ Upstream commit fd98e9419d8d622a4de91f76b306af6aa627aa9c ]
>
> Commit 79901317ce80 ("n_tty: Don't flush buffer when closing ldisc"),
> first merged in kernel release 3.10, caused the following regression
> in the Gigaset M101 driver:
>
>
> Again, I'll just note my objection to this commit log.
>
> This driver was always broken because it never initialized
> tty->receive_room,
> but rather relied on common but not guaranteed circumstances to
> function.
>
> The commit noted simply made the underlying bug more evident, but the
> root cause was from the original merge commit of this driver.
I must admit I still don't understand that objection. The meaning of the
term "regression" is simply that something which previously worked
stopped working. It doesn't imply any statement about the root cause.
The ser-gigaset driver worked before the introduction of commit
79901317ce80. It didn't work anymore after the introduction of that
commit. So it is correct, and does not contradict your statements above
in any way, to state that commit introduced the described regression.
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 3.12 16/33] isdn/gigaset: reset tty->receive_room when attaching ser_gigaset
Am 16.09.2015 um 01:08 schrieb Peter Hurley:
> On Tue, Sep 15, 2015 at 10:22 AM, Jiri Slaby <jsl...@suse.cz
> <mailto:jsl...@suse.cz>> wrote:
>
> From: Tilman Schmidt <til...@imap.cc>
>
> 3.12-stable review patch. If anyone has any objections, please let
> me know.
>
> ===
>
> [ Upstream commit fd98e9419d8d622a4de91f76b306af6aa627aa9c ]
>
> Commit 79901317ce80 ("n_tty: Don't flush buffer when closing ldisc"),
> first merged in kernel release 3.10, caused the following regression
> in the Gigaset M101 driver:
>
>
> Again, I'll just note my objection to this commit log.
>
> This driver was always broken because it never initialized
> tty->receive_room,
> but rather relied on common but not guaranteed circumstances to
> function.
>
> The commit noted simply made the underlying bug more evident, but the
> root cause was from the original merge commit of this driver.
I must admit I still don't understand that objection. The meaning of the
term "regression" is simply that something which previously worked
stopped working. It doesn't imply any statement about the root cause.
The ser-gigaset driver worked before the introduction of commit
79901317ce80. It didn't work anymore after the introduction of that
commit. So it is correct, and does not contradict your statements above
in any way, to state that commit introduced the described regression.
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/2] isdn/gigaset: reset tty->receive_room when attaching ser_gigaset
Am 14.07.2015 um 01:14 schrieb Peter Hurley:
> On 07/13/2015 06:37 PM, Tilman Schmidt wrote:
>> Commit 79901317ce80 ("n_tty: Don't flush buffer when closing ldisc"),
>> first merged in kernel release 3.10, caused the following regression
>> in the Gigaset M101 driver:
>>
>> Before that commit, when closing the N_TTY line discipline in
>> preparation to switching to N_GIGASET_M101, receive_room would be
>> reset to a non-zero value by the call to n_tty_flush_buffer() in
>> n_tty's close method. With the removal of that call, receive_room
>> might be left at zero, blocking data reception on the serial line.
>
> That commit didn't cause the problem; it was a bug all along.
Sure. That's why it is correctly fixed in the Gigaset driver.
But before that commit the bug was never actually triggered.
So that commit defines the point in the commit history from
which the fix is needed, and therefore needs to be mentioned
in order to decide which stable releases will need the fix.
> Non-flow controlling line disciplines _must_ set tty->receive_room
> on line discipline open because they are declaring that every
> input they can accept that much data.
I have submitted a corresponding fix to the line discipline
documentation separately.
Thanks,
Tilman
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] isdn/gigaset: drop unused ldisc methods
The line discipline read and write methods are optional so the dummy
methods in ser_gigaset are unnecessary and can be removed.
Signed-off-by: Tilman Schmidt
---
drivers/isdn/gigaset/ser-gigaset.c | 24
1 files changed, 0 insertions(+), 24 deletions(-)
diff --git a/drivers/isdn/gigaset/ser-gigaset.c
b/drivers/isdn/gigaset/ser-gigaset.c
index 3ac9c41..375be50 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -607,28 +607,6 @@ static int gigaset_tty_hangup(struct tty_struct *tty)
}
/*
- * Read on the tty.
- * Unused, received data goes only to the Gigaset driver.
- */
-static ssize_t
-gigaset_tty_read(struct tty_struct *tty, struct file *file,
-unsigned char __user *buf, size_t count)
-{
- return -EAGAIN;
-}
-
-/*
- * Write on the tty.
- * Unused, transmit data comes only from the Gigaset driver.
- */
-static ssize_t
-gigaset_tty_write(struct tty_struct *tty, struct file *file,
- const unsigned char *buf, size_t count)
-{
- return -EAGAIN;
-}
-
-/*
* Ioctl on the tty.
* Called in process context only.
* May be re-entered by multiple ioctl calling threads.
@@ -761,8 +739,6 @@ static struct tty_ldisc_ops gigaset_ldisc = {
.open = gigaset_tty_open,
.close = gigaset_tty_close,
.hangup = gigaset_tty_hangup,
- .read = gigaset_tty_read,
- .write = gigaset_tty_write,
.ioctl = gigaset_tty_ioctl,
.receive_buf= gigaset_tty_receive,
.write_wakeup = gigaset_tty_wakeup,
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/2] Fix long-standing regression in ser_gigaset ISDN driver
This series fixes a serious regression in the Gigaset M101 driver introduced in kernel release 3.10 and removes some unneeded code. Please also queue up patch 1 of the series for inclusion in the stable/longterm releases 3.10 and later. Tilman Schmidt (2): isdn/gigaset: reset tty->receive_room when attaching ser_gigaset isdn/gigaset: drop unused ldisc methods drivers/isdn/gigaset/ser-gigaset.c | 35 ++- 1 files changed, 10 insertions(+), 25 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] isdn/gigaset: reset tty->receive_room when attaching ser_gigaset
Commit 79901317ce80 ("n_tty: Don't flush buffer when closing ldisc"),
first merged in kernel release 3.10, caused the following regression
in the Gigaset M101 driver:
Before that commit, when closing the N_TTY line discipline in
preparation to switching to N_GIGASET_M101, receive_room would be
reset to a non-zero value by the call to n_tty_flush_buffer() in
n_tty's close method. With the removal of that call, receive_room
might be left at zero, blocking data reception on the serial line.
The present patch fixes that regression by setting receive_room
to an appropriate value in the ldisc open method.
Fixes: 79901317ce80 ("n_tty: Don't flush buffer when closing ldisc")
Signed-off-by: Tilman Schmidt
---
drivers/isdn/gigaset/ser-gigaset.c | 11 ++-
1 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/drivers/isdn/gigaset/ser-gigaset.c
b/drivers/isdn/gigaset/ser-gigaset.c
index 8c91fd5..3ac9c41 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -524,9 +524,18 @@ gigaset_tty_open(struct tty_struct *tty)
cs->hw.ser->tty = tty;
atomic_set(>hw.ser->refcnt, 1);
init_completion(>hw.ser->dead_cmp);
-
tty->disc_data = cs;
+ /* Set the amount of data we're willing to receive per call
+* from the hardware driver to half of the input buffer size
+* to leave some reserve.
+* Note: We don't do flow control towards the hardware driver.
+* If more data is received than will fit into the input buffer,
+* it will be dropped and an error will be logged. This should
+* never happen as the device is slow and the buffer size ample.
+*/
+ tty->receive_room = RBUFSIZE/2;
+
/* OK.. Initialization of the datastructures and the HW is done.. Now
* startup system and notify the LL that we are ready to run
*/
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Documentation: improve line discipline method descriptions
Mention that the ldisc open method must reset tty->receive_room, and that many methods are optional. Add description of receive_buf2 method. Signed-off-by: Tilman Schmidt --- Documentation/serial/tty.txt | 59 +++--- 1 files changed, 38 insertions(+), 21 deletions(-) diff --git a/Documentation/serial/tty.txt b/Documentation/serial/tty.txt index 973c8ad..17624a0 100644 --- a/Documentation/serial/tty.txt +++ b/Documentation/serial/tty.txt @@ -39,8 +39,12 @@ TTY side interfaces: open() - Called when the line discipline is attached to the terminal. No other call into the line discipline for this tty will occur until it - completes successfully. Returning an error will - prevent the ldisc from being attached. Can sleep. + completes successfully. Should initialize any + state needed by the ldisc, and set receive_room + in the tty_struct to an appropriate value to + indicate more data can be sent. Returning an + error will prevent the ldisc from being attached. + May sleep. close()- This is called on a terminal when the line discipline is being unplugged. At the point of @@ -52,9 +56,16 @@ hangup() - Called when the tty line is hung up. No further calls into the ldisc code will occur. The return value is ignored. Can sleep. -write()- A process is writing data through the line - discipline. Multiple write calls are serialized - by the tty layer for the ldisc. May sleep. +read() - (optional) A process requests reading data from + the line. Multiple read calls may occur in parallel + and the ldisc must deal with serialization issues. + If not defined, the process will receive an EIO + error. May sleep. + +write()- (optional) A process requests writing data to the + line. Multiple write calls are serialized by the + tty layer for the ldisc. If not defined, the + process will receive an EIO error. May sleep. flush_buffer() - (optional) May be called at any point between open and close, and instructs the line discipline @@ -69,27 +80,33 @@ set_termios() - (optional) Called on termios structure changes. termios semaphore so allowed to sleep. Serialized against itself only. -read() - Move data from the line discipline to the user. - Multiple read calls may occur in parallel and the - ldisc must deal with serialization issues. May - sleep. - -poll() - Check the status for the poll/select calls. Multiple - poll calls may occur in parallel. May sleep. +poll() - (optional) Check the status for the poll/select + calls. Multiple poll calls may occur in parallel. + May sleep. -ioctl()- Called when an ioctl is handed to the tty layer - that might be for the ldisc. Multiple ioctl calls - may occur in parallel. May sleep. +ioctl()- (optional) Called when an ioctl is handed to the + tty layer that might be for the ldisc. Multiple + ioctl calls may occur in parallel. May sleep. -compat_ioctl() - Called when a 32 bit ioctl is handed to the tty layer - that might be for the ldisc. Multiple ioctl calls - may occur in parallel. May sleep. +compat_ioctl() - (optional) Called when a 32 bit ioctl is handed + to the tty layer that might be for the ldisc. + Multiple ioctl calls may occur in parallel. + May sleep. Driver Side Interfaces: -receive_buf() - Hand buffers of bytes from the driver to the ldisc - for processing. Semantics currently rather - mysterious 8( +receive_buf() - (optional) Called by the low-level driver to hand + a buffer of received bytes to the ldisc for + processing. The number of bytes is guaranteed not + to exceed the current value of tty->receive_room. + All bytes must be processed. + +receive_buf2() - (optional) Called by the low-level driver to hand + a buffer of received
Re: [PATCH 1/2] isdn/gigaset: reset tty-receive_room when attaching ser_gigaset
Am 14.07.2015 um 01:14 schrieb Peter Hurley: On 07/13/2015 06:37 PM, Tilman Schmidt wrote: Commit 79901317ce80 (n_tty: Don't flush buffer when closing ldisc), first merged in kernel release 3.10, caused the following regression in the Gigaset M101 driver: Before that commit, when closing the N_TTY line discipline in preparation to switching to N_GIGASET_M101, receive_room would be reset to a non-zero value by the call to n_tty_flush_buffer() in n_tty's close method. With the removal of that call, receive_room might be left at zero, blocking data reception on the serial line. That commit didn't cause the problem; it was a bug all along. Sure. That's why it is correctly fixed in the Gigaset driver. But before that commit the bug was never actually triggered. So that commit defines the point in the commit history from which the fix is needed, and therefore needs to be mentioned in order to decide which stable releases will need the fix. Non-flow controlling line disciplines _must_ set tty-receive_room on line discipline open because they are declaring that every input they can accept that much data. I have submitted a corresponding fix to the line discipline documentation separately. Thanks, Tilman -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Documentation: improve line discipline method descriptions
Mention that the ldisc open method must reset tty-receive_room, and that many methods are optional. Add description of receive_buf2 method. Signed-off-by: Tilman Schmidt til...@imap.cc --- Documentation/serial/tty.txt | 59 +++--- 1 files changed, 38 insertions(+), 21 deletions(-) diff --git a/Documentation/serial/tty.txt b/Documentation/serial/tty.txt index 973c8ad..17624a0 100644 --- a/Documentation/serial/tty.txt +++ b/Documentation/serial/tty.txt @@ -39,8 +39,12 @@ TTY side interfaces: open() - Called when the line discipline is attached to the terminal. No other call into the line discipline for this tty will occur until it - completes successfully. Returning an error will - prevent the ldisc from being attached. Can sleep. + completes successfully. Should initialize any + state needed by the ldisc, and set receive_room + in the tty_struct to an appropriate value to + indicate more data can be sent. Returning an + error will prevent the ldisc from being attached. + May sleep. close()- This is called on a terminal when the line discipline is being unplugged. At the point of @@ -52,9 +56,16 @@ hangup() - Called when the tty line is hung up. No further calls into the ldisc code will occur. The return value is ignored. Can sleep. -write()- A process is writing data through the line - discipline. Multiple write calls are serialized - by the tty layer for the ldisc. May sleep. +read() - (optional) A process requests reading data from + the line. Multiple read calls may occur in parallel + and the ldisc must deal with serialization issues. + If not defined, the process will receive an EIO + error. May sleep. + +write()- (optional) A process requests writing data to the + line. Multiple write calls are serialized by the + tty layer for the ldisc. If not defined, the + process will receive an EIO error. May sleep. flush_buffer() - (optional) May be called at any point between open and close, and instructs the line discipline @@ -69,27 +80,33 @@ set_termios() - (optional) Called on termios structure changes. termios semaphore so allowed to sleep. Serialized against itself only. -read() - Move data from the line discipline to the user. - Multiple read calls may occur in parallel and the - ldisc must deal with serialization issues. May - sleep. - -poll() - Check the status for the poll/select calls. Multiple - poll calls may occur in parallel. May sleep. +poll() - (optional) Check the status for the poll/select + calls. Multiple poll calls may occur in parallel. + May sleep. -ioctl()- Called when an ioctl is handed to the tty layer - that might be for the ldisc. Multiple ioctl calls - may occur in parallel. May sleep. +ioctl()- (optional) Called when an ioctl is handed to the + tty layer that might be for the ldisc. Multiple + ioctl calls may occur in parallel. May sleep. -compat_ioctl() - Called when a 32 bit ioctl is handed to the tty layer - that might be for the ldisc. Multiple ioctl calls - may occur in parallel. May sleep. +compat_ioctl() - (optional) Called when a 32 bit ioctl is handed + to the tty layer that might be for the ldisc. + Multiple ioctl calls may occur in parallel. + May sleep. Driver Side Interfaces: -receive_buf() - Hand buffers of bytes from the driver to the ldisc - for processing. Semantics currently rather - mysterious 8( +receive_buf() - (optional) Called by the low-level driver to hand + a buffer of received bytes to the ldisc for + processing. The number of bytes is guaranteed not + to exceed the current value of tty-receive_room. + All bytes must be processed. + +receive_buf2() - (optional) Called by the low-level driver to hand + a buffer
[PATCH 0/2] Fix long-standing regression in ser_gigaset ISDN driver
This series fixes a serious regression in the Gigaset M101 driver introduced in kernel release 3.10 and removes some unneeded code. Please also queue up patch 1 of the series for inclusion in the stable/longterm releases 3.10 and later. Tilman Schmidt (2): isdn/gigaset: reset tty-receive_room when attaching ser_gigaset isdn/gigaset: drop unused ldisc methods drivers/isdn/gigaset/ser-gigaset.c | 35 ++- 1 files changed, 10 insertions(+), 25 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] isdn/gigaset: drop unused ldisc methods
The line discipline read and write methods are optional so the dummy
methods in ser_gigaset are unnecessary and can be removed.
Signed-off-by: Tilman Schmidt til...@imap.cc
---
drivers/isdn/gigaset/ser-gigaset.c | 24
1 files changed, 0 insertions(+), 24 deletions(-)
diff --git a/drivers/isdn/gigaset/ser-gigaset.c
b/drivers/isdn/gigaset/ser-gigaset.c
index 3ac9c41..375be50 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -607,28 +607,6 @@ static int gigaset_tty_hangup(struct tty_struct *tty)
}
/*
- * Read on the tty.
- * Unused, received data goes only to the Gigaset driver.
- */
-static ssize_t
-gigaset_tty_read(struct tty_struct *tty, struct file *file,
-unsigned char __user *buf, size_t count)
-{
- return -EAGAIN;
-}
-
-/*
- * Write on the tty.
- * Unused, transmit data comes only from the Gigaset driver.
- */
-static ssize_t
-gigaset_tty_write(struct tty_struct *tty, struct file *file,
- const unsigned char *buf, size_t count)
-{
- return -EAGAIN;
-}
-
-/*
* Ioctl on the tty.
* Called in process context only.
* May be re-entered by multiple ioctl calling threads.
@@ -761,8 +739,6 @@ static struct tty_ldisc_ops gigaset_ldisc = {
.open = gigaset_tty_open,
.close = gigaset_tty_close,
.hangup = gigaset_tty_hangup,
- .read = gigaset_tty_read,
- .write = gigaset_tty_write,
.ioctl = gigaset_tty_ioctl,
.receive_buf= gigaset_tty_receive,
.write_wakeup = gigaset_tty_wakeup,
--
1.7.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] isdn/gigaset: reset tty-receive_room when attaching ser_gigaset
Commit 79901317ce80 (n_tty: Don't flush buffer when closing ldisc), first merged in kernel release 3.10, caused the following regression in the Gigaset M101 driver: Before that commit, when closing the N_TTY line discipline in preparation to switching to N_GIGASET_M101, receive_room would be reset to a non-zero value by the call to n_tty_flush_buffer() in n_tty's close method. With the removal of that call, receive_room might be left at zero, blocking data reception on the serial line. The present patch fixes that regression by setting receive_room to an appropriate value in the ldisc open method. Fixes: 79901317ce80 (n_tty: Don't flush buffer when closing ldisc) Signed-off-by: Tilman Schmidt til...@imap.cc --- drivers/isdn/gigaset/ser-gigaset.c | 11 ++- 1 files changed, 10 insertions(+), 1 deletions(-) diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c index 8c91fd5..3ac9c41 100644 --- a/drivers/isdn/gigaset/ser-gigaset.c +++ b/drivers/isdn/gigaset/ser-gigaset.c @@ -524,9 +524,18 @@ gigaset_tty_open(struct tty_struct *tty) cs-hw.ser-tty = tty; atomic_set(cs-hw.ser-refcnt, 1); init_completion(cs-hw.ser-dead_cmp); - tty-disc_data = cs; + /* Set the amount of data we're willing to receive per call +* from the hardware driver to half of the input buffer size +* to leave some reserve. +* Note: We don't do flow control towards the hardware driver. +* If more data is received than will fit into the input buffer, +* it will be dropped and an error will be logged. This should +* never happen as the device is slow and the buffer size ample. +*/ + tty-receive_room = RBUFSIZE/2; + /* OK.. Initialization of the datastructures and the HW is done.. Now * startup system and notify the LL that we are ready to run */ -- 1.7.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: isdn: pcbit: another off-by-one issue?
Am 11.06.2015 um 13:44 schrieb Paul Bolle: > Besides, it is, apparently, an I4L driver. (Though there's a CAPI part > too. I need to check how that fits into the picture.) If I read the code correctly it talks I4L to the kernel but CAPI to the card. This would have been a natural candidate for porting to the kernel CAPI interface, had anyone bothered. Which reinforces the impression that no-one is using this anymore. Note also that the FTP URL mentioned in Documentation/isdn/README.pcbit doesn't exist anymore. -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: isdn: pcbit: another off-by-one issue?
Am 11.06.2015 um 13:44 schrieb Paul Bolle: Besides, it is, apparently, an I4L driver. (Though there's a CAPI part too. I need to check how that fits into the picture.) If I read the code correctly it talks I4L to the kernel but CAPI to the card. This would have been a natural candidate for porting to the kernel CAPI interface, had anyone bothered. Which reinforces the impression that no-one is using this anymore. Note also that the FTP URL mentioned in Documentation/isdn/README.pcbit doesn't exist anymore. -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
[PATCH] isdn/gigaset: cede maintainership
As German phone operators are discontinuing ISDN service, neither Hansjörg nor I will be able to maintain the Gigaset ISDN drivers any longer. Paul Bolle offered to step into the breach for odd fixes. Signed-off-by: Tilman Schmidt Acked-by: Paul Bolle --- MAINTAINERS | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 2e5bbc0..718282d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4364,11 +4364,10 @@ F: fs/gfs2/ F: include/uapi/linux/gfs2_ondisk.h GIGASET ISDN DRIVERS -M: Hansjoerg Lipp -M: Tilman Schmidt +M: Paul Bolle L: gigaset307x-com...@lists.sourceforge.net W: http://gigaset307x.sourceforge.net/ -S: Maintained +S: Odd Fixes F: Documentation/isdn/README.gigaset F: drivers/isdn/gigaset/ F: include/uapi/linux/gigaset_dev.h -- 1.9.2.459.g68773ac -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] isdn/gigaset: cede maintainership
As German phone operators are discontinuing ISDN service, neither Hansjörg nor I will be able to maintain the Gigaset ISDN drivers any longer. Paul Bolle offered to step into the breach for odd fixes. Signed-off-by: Tilman Schmidt til...@imap.cc Acked-by: Paul Bolle pebo...@tiscali.nl --- MAINTAINERS | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 2e5bbc0..718282d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4364,11 +4364,10 @@ F: fs/gfs2/ F: include/uapi/linux/gfs2_ondisk.h GIGASET ISDN DRIVERS -M: Hansjoerg Lipp hjl...@web.de -M: Tilman Schmidt til...@imap.cc +M: Paul Bolle pebo...@tiscali.nl L: gigaset307x-com...@lists.sourceforge.net W: http://gigaset307x.sourceforge.net/ -S: Maintained +S: Odd Fixes F: Documentation/isdn/README.gigaset F: drivers/isdn/gigaset/ F: include/uapi/linux/gigaset_dev.h -- 1.9.2.459.g68773ac -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kill I4L?
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Am 09.02.2015 um 11:07 schrieb Bas Peters: > 2015-02-09 0:59 GMT+01:00 Karsten Keil : >> Am 08.02.2015 um 20:47 schrieb Tilman Schmidt: >>> Am 07.02.2015 um 21:43 schrieb Paul Bolle: >>>> [M]aybe we should consider, say, removing i4l and pre i4l and >>>> see who complains. That might be a rude thing to do. So >>>> perhaps the various ISDN flavors should be left alone until >>>> ... what exactly? >>> >>> I'd support that step. I don't think it'll hurt anyone because >>> the cards supported by i4l are mostly ISA cards anyway. The >>> only exceptions are the HiSax family which is now supported by >>> mISDN, and the Hypercope family which is supported by CAPI. [...] >> >> But I4L is still the default in some Distros, so we should allow >> a warning period. But again, I'm fine with this to do it. > > Is there any explicit reason why 'dead' drivers that might still > have some users ought to be removed? The reason is the maintenance load it produces. There's a continuous, annoying trickle of patch proposals, discussions, conflicts with development in other, still actively maintained areas of the kernel, and so on. The present discussion being a point in case. > Does it hurt anyone to leave the code in there, despite it barely > being used? Yes it does. Not much, but the pain is increasing over the years. Every time someone tries to touch that code there's the problem that no one can actually answer for it, much less test anything. Theoretically a patch for a driver should not be accepted without testing it on the actual hardware, but in the isdn tree that rule has long been abandoned because nobody has the hardware and can do the test. Consequently it isn't even clear whether all of it still actually works. It also hurts the few remaining Linux ISDN developers, distros and users that Linux ISDN support is so fragmented. For example, the Gigaset ISDN driver which I maintain can be built with either CAPI or I4L support, so each time I touch it I have to build and test two variants. > We're not talking about a particularly huge driver here, either. But one that's particularly difficult to maintain, without providing any noticeable benefit in return. - -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) -BEGIN PGP SIGNATURE- Version: GnuPG v2 iQEcBAEBAgAGBQJU2JHAAAoJEFPuqx0v+F+qn5EH/0iXrzTWChbu/0W8nDz4/qtC FWQYbeIxTutzsEtVAhOYM7mz+9mqgaqNkVpmrz4lLg3FY4q2kzG1GjSihqP0GsT+ rLWJ+7gTnNxjNOk6OOZo+GaOjcvtVAro/2N5NXhHxTseumbH4I371a2rw0HBls97 iCPB2g6mJvNnsLjb612qcgsGahxMWVE/3q+6O1IKujPCTNQsJNaeqQMPT3YFJwq+ 4YMs55RpVbpP5GPdRsaW/Zkwx8Se/4cK1MFaqX9xEePgZDUYMCPT2BPEa7E3yUwF kjJU5LnBTKAjI8IzXDPTzznAyrMnH6IAjtJSmwpnyNintv2dtCK0VPhjhh0TA/M= =d5Or -END PGP SIGNATURE- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Kill I4L?
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Am 09.02.2015 um 11:07 schrieb Bas Peters: 2015-02-09 0:59 GMT+01:00 Karsten Keil kk...@linux-pingi.de: Am 08.02.2015 um 20:47 schrieb Tilman Schmidt: Am 07.02.2015 um 21:43 schrieb Paul Bolle: [M]aybe we should consider, say, removing i4l and pre i4l and see who complains. That might be a rude thing to do. So perhaps the various ISDN flavors should be left alone until ... what exactly? I'd support that step. I don't think it'll hurt anyone because the cards supported by i4l are mostly ISA cards anyway. The only exceptions are the HiSax family which is now supported by mISDN, and the Hypercope family which is supported by CAPI. [...] But I4L is still the default in some Distros, so we should allow a warning period. But again, I'm fine with this to do it. Is there any explicit reason why 'dead' drivers that might still have some users ought to be removed? The reason is the maintenance load it produces. There's a continuous, annoying trickle of patch proposals, discussions, conflicts with development in other, still actively maintained areas of the kernel, and so on. The present discussion being a point in case. Does it hurt anyone to leave the code in there, despite it barely being used? Yes it does. Not much, but the pain is increasing over the years. Every time someone tries to touch that code there's the problem that no one can actually answer for it, much less test anything. Theoretically a patch for a driver should not be accepted without testing it on the actual hardware, but in the isdn tree that rule has long been abandoned because nobody has the hardware and can do the test. Consequently it isn't even clear whether all of it still actually works. It also hurts the few remaining Linux ISDN developers, distros and users that Linux ISDN support is so fragmented. For example, the Gigaset ISDN driver which I maintain can be built with either CAPI or I4L support, so each time I touch it I have to build and test two variants. We're not talking about a particularly huge driver here, either. But one that's particularly difficult to maintain, without providing any noticeable benefit in return. - -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) -BEGIN PGP SIGNATURE- Version: GnuPG v2 iQEcBAEBAgAGBQJU2JHAAAoJEFPuqx0v+F+qn5EH/0iXrzTWChbu/0W8nDz4/qtC FWQYbeIxTutzsEtVAhOYM7mz+9mqgaqNkVpmrz4lLg3FY4q2kzG1GjSihqP0GsT+ rLWJ+7gTnNxjNOk6OOZo+GaOjcvtVAro/2N5NXhHxTseumbH4I371a2rw0HBls97 iCPB2g6mJvNnsLjb612qcgsGahxMWVE/3q+6O1IKujPCTNQsJNaeqQMPT3YFJwq+ 4YMs55RpVbpP5GPdRsaW/Zkwx8Se/4cK1MFaqX9xEePgZDUYMCPT2BPEa7E3yUwF kjJU5LnBTKAjI8IzXDPTzznAyrMnH6IAjtJSmwpnyNintv2dtCK0VPhjhh0TA/M= =d5Or -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Kill I4L? (was: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors)
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Am 07.02.2015 um 21:43 schrieb Paul Bolle: > On Sat, 2015-02-07 at 11:19 -0800, Joe Perches wrote: >> Does anyone still use these cards? > > 0) Good question. I very much doubt it. It was an ISA card, not even PnP. I'd imagine any systems of that kind would be retired by now. > 2) Broader picture: if I remember correctly there are now four > different flavors of ISDN in the kernel: - really old: pre-i4l I don't think any traces of that are still present in current kernel releases. It should all have been left behind on the switch to 2.6. > - very old: i4l - just old: CAPI - not so old: mISDN Those are the in-tree ones. To make matters worse, the Asterisk people invented three more (Zaptel, DAHDI and vISDN) which are maintained out-of-tree. Apparently they weren't satisfied with any the in-tree ones and didn't feel like helping to improve one of them to match their needs. > [snip] So the current ISDN situation is a bit messy. That's putting it mildly. > Tilman might be able to provide a clearer, and maybe less grumpy, > summary of the current situation. Don't know about either. ;-) > [M]aybe we should consider, say, removing i4l and pre i4l and see > who complains. That might be a rude thing to do. So perhaps the > various ISDN flavors should be left alone until ... what exactly? I'd support that step. I don't think it'll hurt anyone because the cards supported by i4l are mostly ISA cards anyway. The only exceptions are the HiSax family which is now supported by mISDN, and the Hypercope family which is supported by CAPI. In the past we had Documentation/feature-removal-schedule.txt for announcing removals like that but Linus shot that down on 2012-10-01 (commit 9c0ece069b32e8e122aea71aa47181c10eb85ba7) so I guess somebody could just submit a patch series starting with - --- a/drivers/isdn/Kconfig +++ b/drivers/isdn/Kconfig @@ -20,25 +20,6 @@ menuconfig ISDN if ISDN - -menuconfig ISDN_I4L - - tristate "Old ISDN4Linux (deprecated)" - - depends on TTY - - ---help--- - - This driver allows you to use an ISDN adapter for networking - - connections and as dialin/out device. The isdn-tty's have a built - - in AT-compatible modem emulator. Network devices support autodial, - - channel-bundling, callback and caller-authentication without having - - a daemon running. A reduced T.70 protocol is supported with tty's - - suitable for German BTX. On D-Channel, the protocols EDSS1 - - (Euro-ISDN) and 1TR6 (German style) are supported. See - - for more information. - - - - ISDN support in the linux kernel is moving towards a new API, - - called CAPI (Common ISDN Application Programming Interface). - - Therefore the old ISDN4Linux layer will eventually become obsolete. - - It is still available, though, for use with adapters that are not - - supported by the new CAPI subsystem yet. - - source "drivers/isdn/i4l/Kconfig" menuconfig ISDN_CAPI and working its way from that to remove anything that's become unreachable. Shall I? - -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) -BEGIN PGP SIGNATURE- Version: GnuPG v2 iQEcBAEBAgAGBQJU171KAAoJEFPuqx0v+F+qnqgH/3KCC6wnXWKOxfuWZ13lwqa2 GuFy+DMYZbXi5e5IHIYnTO9LvA7uzg4ryokP4iMWazhIpx5Cx0Riil59LJExE59p jsLizpDeZ+z+wwrEZHimc8lYEFO70abvgKL5NdkF8luc+QPIJAmDyLA+8c6J0rKo VcIb+vq3hW06L7FeKD3Y03NU1xZDtG6HxhnCQRV5b8Ve6sfeu4Oz/83yLyvvfNbK mnRWbUisO2TmBWcTbZ0QB889iPBRrR1HD4TLA9WZ38fQs7YbuKNbz4jxMKDDSh2i JHDK8tzn5P+73CPHkvRfELODn+V5uzUWh6QHJFD+4sMOKvNZfbxlEmt1GANmto0= =PtE1 -END PGP SIGNATURE- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Kill I4L? (was: [PATCH 2/6] drivers: isdn: act2000: capi.c: fix checkpatch errors)
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Am 07.02.2015 um 21:43 schrieb Paul Bolle: On Sat, 2015-02-07 at 11:19 -0800, Joe Perches wrote: Does anyone still use these cards? 0) Good question. I very much doubt it. It was an ISA card, not even PnP. I'd imagine any systems of that kind would be retired by now. 2) Broader picture: if I remember correctly there are now four different flavors of ISDN in the kernel: - really old: pre-i4l I don't think any traces of that are still present in current kernel releases. It should all have been left behind on the switch to 2.6. - very old: i4l - just old: CAPI - not so old: mISDN Those are the in-tree ones. To make matters worse, the Asterisk people invented three more (Zaptel, DAHDI and vISDN) which are maintained out-of-tree. Apparently they weren't satisfied with any the in-tree ones and didn't feel like helping to improve one of them to match their needs. [snip] So the current ISDN situation is a bit messy. That's putting it mildly. Tilman might be able to provide a clearer, and maybe less grumpy, summary of the current situation. Don't know about either. ;-) [M]aybe we should consider, say, removing i4l and pre i4l and see who complains. That might be a rude thing to do. So perhaps the various ISDN flavors should be left alone until ... what exactly? I'd support that step. I don't think it'll hurt anyone because the cards supported by i4l are mostly ISA cards anyway. The only exceptions are the HiSax family which is now supported by mISDN, and the Hypercope family which is supported by CAPI. In the past we had Documentation/feature-removal-schedule.txt for announcing removals like that but Linus shot that down on 2012-10-01 (commit 9c0ece069b32e8e122aea71aa47181c10eb85ba7) so I guess somebody could just submit a patch series starting with - --- a/drivers/isdn/Kconfig +++ b/drivers/isdn/Kconfig @@ -20,25 +20,6 @@ menuconfig ISDN if ISDN - -menuconfig ISDN_I4L - - tristate Old ISDN4Linux (deprecated) - - depends on TTY - - ---help--- - - This driver allows you to use an ISDN adapter for networking - - connections and as dialin/out device. The isdn-tty's have a built - - in AT-compatible modem emulator. Network devices support autodial, - - channel-bundling, callback and caller-authentication without having - - a daemon running. A reduced T.70 protocol is supported with tty's - - suitable for German BTX. On D-Channel, the protocols EDSS1 - - (Euro-ISDN) and 1TR6 (German style) are supported. See - - file:Documentation/isdn/README for more information. - - - - ISDN support in the linux kernel is moving towards a new API, - - called CAPI (Common ISDN Application Programming Interface). - - Therefore the old ISDN4Linux layer will eventually become obsolete. - - It is still available, though, for use with adapters that are not - - supported by the new CAPI subsystem yet. - - source drivers/isdn/i4l/Kconfig menuconfig ISDN_CAPI and working its way from that to remove anything that's become unreachable. Shall I? - -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) -BEGIN PGP SIGNATURE- Version: GnuPG v2 iQEcBAEBAgAGBQJU171KAAoJEFPuqx0v+F+qnqgH/3KCC6wnXWKOxfuWZ13lwqa2 GuFy+DMYZbXi5e5IHIYnTO9LvA7uzg4ryokP4iMWazhIpx5Cx0Riil59LJExE59p jsLizpDeZ+z+wwrEZHimc8lYEFO70abvgKL5NdkF8luc+QPIJAmDyLA+8c6J0rKo VcIb+vq3hW06L7FeKD3Y03NU1xZDtG6HxhnCQRV5b8Ve6sfeu4Oz/83yLyvvfNbK mnRWbUisO2TmBWcTbZ0QB889iPBRrR1HD4TLA9WZ38fQs7YbuKNbz4jxMKDDSh2i JHDK8tzn5P+73CPHkvRfELODn+V5uzUWh6QHJFD+4sMOKvNZfbxlEmt1GANmto0= =PtE1 -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: make allyesconfig i386 build failure with next-20150122 (caused by fb_agm1264k-fl driver)
Am 27.01.2015 um 03:42 schrieb Guenter Roeck: > On 01/26/2015 02:46 PM, Greg Kroah-Hartman wrote: >> On Mon, Jan 26, 2015 at 01:59:59PM -0800, Guenter Roeck wrote: >>> On Thu, Jan 22, 2015 at 12:10:33PM -0700, Jim Davis wrote: make ARCH=i386 allyesconfig fails with drivers/staging/built-in.o: In function `reset': (.text+0x2ae89d): multiple definition of `reset' drivers/isdn/built-in.o:(.text+0x185dc2): first defined here make[1]: *** [drivers/built-in.o] Error 1 >>> >>> Culprit: >>> >>> commit b2ebd4be6fa1d2329b63531b044f9e25474981cb >>> Author: Thomas Petazzoni >>> Date: Wed Dec 31 10:11:10 2014 +0100 >>> >>> staging: fbtft: add fb_agm1264k-fl driver >>> >>> A global function named 'reset' isn't really a good idea. >>> >>> Not that the global function with the same name in the isdn code >>> is better ;-). >> >> Agreed, the fbtft code is now fixed. Patches to fix the isdn code would >> be gladly accepted as well :) >> > > Did you have a look into the isdn code ? It will need someone > with a lot of spare time to clean this code up. Cleaning up the entire isdn code certainly will, but that particular problem is in fact much more limited. The function in question is declared, along with several other also much too generically named ones, in drivers/isdn/sc/card.h which is included nowhere outside drivers/isdn/sc. So these are really private functions of the SpellCaster driver which could easily be renamed. OTOH good old SpellCaster was an ISA card, and the sc driver depends on the deprecated I4L framework. I wonder if it's worth any effort. Tilman -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: make allyesconfig i386 build failure with next-20150122 (caused by fb_agm1264k-fl driver)
Am 27.01.2015 um 03:42 schrieb Guenter Roeck: On 01/26/2015 02:46 PM, Greg Kroah-Hartman wrote: On Mon, Jan 26, 2015 at 01:59:59PM -0800, Guenter Roeck wrote: On Thu, Jan 22, 2015 at 12:10:33PM -0700, Jim Davis wrote: make ARCH=i386 allyesconfig fails with drivers/staging/built-in.o: In function `reset': (.text+0x2ae89d): multiple definition of `reset' drivers/isdn/built-in.o:(.text+0x185dc2): first defined here make[1]: *** [drivers/built-in.o] Error 1 Culprit: commit b2ebd4be6fa1d2329b63531b044f9e25474981cb Author: Thomas Petazzoni thomas.petazz...@free-electrons.com Date: Wed Dec 31 10:11:10 2014 +0100 staging: fbtft: add fb_agm1264k-fl driver A global function named 'reset' isn't really a good idea. Not that the global function with the same name in the isdn code is better ;-). Agreed, the fbtft code is now fixed. Patches to fix the isdn code would be gladly accepted as well :) Did you have a look into the isdn code ? It will need someone with a lot of spare time to clean this code up. Cleaning up the entire isdn code certainly will, but that particular problem is in fact much more limited. The function in question is declared, along with several other also much too generically named ones, in drivers/isdn/sc/card.h which is included nowhere outside drivers/isdn/sc. So these are really private functions of the SpellCaster driver which could easily be renamed. OTOH good old SpellCaster was an ISA card, and the sc driver depends on the deprecated I4L framework. I wonder if it's worth any effort. Tilman -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] isdn:gigaset:Remove unneeded TODO comment for the function,gigaset_procinfo in capi.c
Am 17.01.2015 um 19:23 schrieb Nicholas Krause:
> This removes the no longer needed TODO comment for questioning if
> the function,gigaset_procinfo needs to do more work internally. This
> comment is no longer needed due to the function's intended use to be
> returning the name of the structure pointer passed to this function
> of type,capi_ctr. In addition due to this the function's comments
> related to it's intended job are changed to state it returns rather
> then builds the description for the structure and also that the
> function doesn't build the name string but returns the already
> generated one passed to gigaset_procinfo by the structure passed
> to this function.
>
> Signed-off-by: Nicholas Krause
Acked-by: Tilman Schmidt
Thanks,
Tilman
> ---
> drivers/isdn/gigaset/capi.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/isdn/gigaset/capi.c b/drivers/isdn/gigaset/capi.c
> index ccec777..c4d666e 100644
> --- a/drivers/isdn/gigaset/capi.c
> +++ b/drivers/isdn/gigaset/capi.c
> @@ -2336,14 +2336,14 @@ static u16 gigaset_send_message(struct capi_ctr *ctr,
> struct sk_buff *skb)
> }
>
> /**
> - * gigaset_procinfo() - build single line description for controller
> + * gigaset_procinfo() - return single line description for controller
> * @ctr: controller descriptor structure.
> *
> - * Return value: pointer to generated string (null terminated)
> + * Return value: pointer to already generated string (null terminated)
> */
> static char *gigaset_procinfo(struct capi_ctr *ctr)
> {
> - return ctr->name; /* ToDo: more? */
> + return ctr->name;
> }
>
> static int gigaset_proc_show(struct seq_file *m, void *v)
>
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] isdn:gigaset:Remove unneeded TODO comment for the function,gigaset_procinfo in capi.c
Am 17.01.2015 um 19:23 schrieb Nicholas Krause:
This removes the no longer needed TODO comment for questioning if
the function,gigaset_procinfo needs to do more work internally. This
comment is no longer needed due to the function's intended use to be
returning the name of the structure pointer passed to this function
of type,capi_ctr. In addition due to this the function's comments
related to it's intended job are changed to state it returns rather
then builds the description for the structure and also that the
function doesn't build the name string but returns the already
generated one passed to gigaset_procinfo by the structure passed
to this function.
Signed-off-by: Nicholas Krause xerofo...@gmail.com
Acked-by: Tilman Schmidt til...@imap.cc
Thanks,
Tilman
---
drivers/isdn/gigaset/capi.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/isdn/gigaset/capi.c b/drivers/isdn/gigaset/capi.c
index ccec777..c4d666e 100644
--- a/drivers/isdn/gigaset/capi.c
+++ b/drivers/isdn/gigaset/capi.c
@@ -2336,14 +2336,14 @@ static u16 gigaset_send_message(struct capi_ctr *ctr,
struct sk_buff *skb)
}
/**
- * gigaset_procinfo() - build single line description for controller
+ * gigaset_procinfo() - return single line description for controller
* @ctr: controller descriptor structure.
*
- * Return value: pointer to generated string (null terminated)
+ * Return value: pointer to already generated string (null terminated)
*/
static char *gigaset_procinfo(struct capi_ctr *ctr)
{
- return ctr-name; /* ToDo: more? */
+ return ctr-name;
}
static int gigaset_proc_show(struct seq_file *m, void *v)
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Drivers: isdn: gigaset: checkpatch cleanup
eak;
> + }
> + if (val == -ERANGE) {
> + dev_err(cs->dev, "Overflow error converting
> string to\
> + unsigned long\n");
> + break;
> + }
> + if (val > INT_MAX || *e == s)
> break;
> if (i == 3) {
> if (*e)
> @@ -1364,7 +1372,7 @@ static void do_action(int action, struct cardstate *cs,
> } else if (*e != '.')
> break;
> else
> - s = e + 1;
> + s = *e + 1;
> cs->fwver[i] = val;
> }
> if (i != 4) {
This cannot work. The pointer variable e gets dereferenced without
ever being initialized. The type mismatches when declaring e as
pointing to an unsigned long but comparing *e to s in one place and to
a character literal in another point make me wonder which semantics
you had in mind for e in the first place.
Also your error messages are not helpful for someone reading the log
and trying to find out what went wrong, and not very readable because
of the big stretch of whitespace you insert between the words "to" and
"unsigned". In fact I'm not even convinced it's a good idea to emit a
log message at all here.
> --- a/drivers/isdn/gigaset/gigaset.h
> +++ b/drivers/isdn/gigaset/gigaset.h
> @@ -94,8 +94,7 @@ enum debuglevel {
> #define gig_dbg(level, format, arg...)
> \
> do {\
> if (unlikely(((enum debuglevel)gigaset_debuglevel) & (level))) \
> - printk(KERN_DEBUG KBUILD_MODNAME ": " format "\n", \
> -## arg); \
> + dev_dbg(cs->dev, KBUILD_MODNAME ": " format "\n")\
> } while (0)
This will not work when
- there is no cs variable in the context where the macro is used or
- cs->dev doesn't contain a valid device pointer or
- the format string references additional arguments,
all of which actually occur in the driver.
> --- a/drivers/isdn/gigaset/i4l.c
> +++ b/drivers/isdn/gigaset/i4l.c
> @@ -624,14 +624,14 @@ int gigaset_isdn_regdev(struct cardstate *cs, const
> char *isdnid)
> {
> isdn_if *iif;
>
> - iif = kmalloc(sizeof *iif, GFP_KERNEL);
> + iif = kmalloc(sizeof(*iif, GFP_KERNEL));
You're calling kmalloc with too few arguments here.
> if (!iif) {
> pr_err("out of memory\n");
> return -ENOMEM;
> }
>
> - if (snprintf(iif->id, sizeof iif->id, "%s_%u", isdnid, cs->minor_index)
> - >= sizeof iif->id) {
> + if (snprintf(iif->id, sizeof(iif->id, "%s_%u", isdnid, cs->minor_index))
> + >= sizeof(iif->id)) {
You're calling snprintf with too few arguments here.
> --- a/drivers/isdn/gigaset/proc.c
> +++ b/drivers/isdn/gigaset/proc.c
> @@ -27,13 +27,18 @@ static ssize_t set_cidmode(struct device *dev, struct
> device_attribute *attr,
> const char *buf, size_t count)
> {
> struct cardstate *cs = dev_get_drvdata(dev);
> - long int value;
> - char *end;
> + long int *value;
> + int result;
>
> - value = simple_strtol(buf, , 0);
> - while (*end)
> - if (!isspace(*end++))
> - return -EINVAL;
> + result = kstrtol(buf, 0, );
> + if (result == -ERANGE)
> + /* Overflow error */
> + dev_err(cs->dev, "Overflow error on conversion from string to\
> + long\n");
> + if (result == -EINVAL)
> + /* Parsing error */
> + dev_err(cs->dev, "Parsing error on conversion from string to\
> + long\n");
> if (value < 0 || value > 1)
> return -EINVAL;
>
This changes semantics. Your code will not accept the same input as
the original code, and it will emit messages of its own instead of
just returning an error code to the caller as it should.
In sum: NACK.
Regards,
Tilman
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Drivers: isdn: gigaset: checkpatch cleanup
,
} else if (*e != '.')
break;
else
- s = e + 1;
+ s = *e + 1;
cs-fwver[i] = val;
}
if (i != 4) {
This cannot work. The pointer variable e gets dereferenced without
ever being initialized. The type mismatches when declaring e as
pointing to an unsigned long but comparing *e to s in one place and to
a character literal in another point make me wonder which semantics
you had in mind for e in the first place.
Also your error messages are not helpful for someone reading the log
and trying to find out what went wrong, and not very readable because
of the big stretch of whitespace you insert between the words to and
unsigned. In fact I'm not even convinced it's a good idea to emit a
log message at all here.
--- a/drivers/isdn/gigaset/gigaset.h
+++ b/drivers/isdn/gigaset/gigaset.h
@@ -94,8 +94,7 @@ enum debuglevel {
#define gig_dbg(level, format, arg...)
\
do {\
if (unlikely(((enum debuglevel)gigaset_debuglevel) (level))) \
- printk(KERN_DEBUG KBUILD_MODNAME : format \n, \
-## arg); \
+ dev_dbg(cs-dev, KBUILD_MODNAME : format \n)\
} while (0)
This will not work when
- there is no cs variable in the context where the macro is used or
- cs-dev doesn't contain a valid device pointer or
- the format string references additional arguments,
all of which actually occur in the driver.
--- a/drivers/isdn/gigaset/i4l.c
+++ b/drivers/isdn/gigaset/i4l.c
@@ -624,14 +624,14 @@ int gigaset_isdn_regdev(struct cardstate *cs, const
char *isdnid)
{
isdn_if *iif;
- iif = kmalloc(sizeof *iif, GFP_KERNEL);
+ iif = kmalloc(sizeof(*iif, GFP_KERNEL));
You're calling kmalloc with too few arguments here.
if (!iif) {
pr_err(out of memory\n);
return -ENOMEM;
}
- if (snprintf(iif-id, sizeof iif-id, %s_%u, isdnid, cs-minor_index)
- = sizeof iif-id) {
+ if (snprintf(iif-id, sizeof(iif-id, %s_%u, isdnid, cs-minor_index))
+ = sizeof(iif-id)) {
You're calling snprintf with too few arguments here.
--- a/drivers/isdn/gigaset/proc.c
+++ b/drivers/isdn/gigaset/proc.c
@@ -27,13 +27,18 @@ static ssize_t set_cidmode(struct device *dev, struct
device_attribute *attr,
const char *buf, size_t count)
{
struct cardstate *cs = dev_get_drvdata(dev);
- long int value;
- char *end;
+ long int *value;
+ int result;
- value = simple_strtol(buf, end, 0);
- while (*end)
- if (!isspace(*end++))
- return -EINVAL;
+ result = kstrtol(buf, 0, value);
+ if (result == -ERANGE)
+ /* Overflow error */
+ dev_err(cs-dev, Overflow error on conversion from string to\
+ long\n);
+ if (result == -EINVAL)
+ /* Parsing error */
+ dev_err(cs-dev, Parsing error on conversion from string to\
+ long\n);
if (value 0 || value 1)
return -EINVAL;
This changes semantics. Your code will not accept the same input as
the original code, and it will emit messages of its own instead of
just returning an error code to the caller as it should.
In sum: NACK.
Regards,
Tilman
--
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers:isdn: Remove uneeded fix me comment in capi.c for the function,decode_ie
Am 02.01.2015 um 02:57 schrieb nick: > In that case has anyone tested this on actual hardware supported to see if > removing these two lines breaks anything. If not I feel it's best to just > leave the comment for now. I haven't tested it. The comment was intended as a reminder to myself to do so eventually but obviously didn't succeed. :-) So let's leave it for now. Thanks, Tilman -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers:isdn: Remove uneeded fix me comment in capi.c for the function,decode_ie
Am 02.01.2015 um 02:57 schrieb nick: In that case has anyone tested this on actual hardware supported to see if removing these two lines breaks anything. If not I feel it's best to just leave the comment for now. I haven't tested it. The comment was intended as a reminder to myself to do so eventually but obviously didn't succeed. :-) So let's leave it for now. Thanks, Tilman -- Tilman Schmidt E-Mail: til...@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers:isdn: Remove uneeded fix me comment in capi.c for the function,decode_ie
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
Nicholas,
Am 31.12.2014 um 20:21 schrieb Nicholas Krause:
> Removes a no longer needed fix me comment for the
> function,decode_ie in the file, capi.c. This comment is no longer
> needed as the commit this was written in was 2009 when the new
> driver,three was introduced. Due to no breakage related to the
> newer version of this driver we can safely remove this comment.
I won't oppose removal of that comment. However your commit message is
not quite to the point.
The comment poses the question of whether the conversion to upper case
done by the toupper() calls in the two lines following is actually
necessary. The fact that there is no breakage with the current code
does not answer that question. It could only be answered by *removing*
the toupper() calls and *then* checking for breakage.
However the cost of two toupper() calls seems so small that it's
hardly worth the effort and risk and we should just leave them in.
Regards,
Tilman
> diff --git a/drivers/isdn/gigaset/capi.c
> b/drivers/isdn/gigaset/capi.c index ccec777..d55ba3f 100644 ---
> a/drivers/isdn/gigaset/capi.c +++ b/drivers/isdn/gigaset/capi.c @@
> -182,7 +182,6 @@ static void decode_ie(u8 *in, char *out) { int i =
> *in; while (i-- > 0) { - /* ToDo: conversion to upper case
> necessary? */ *out++ = toupper(hex_asc_hi(*++in)); *out++ =
> toupper(hex_asc_lo(*in)); }
>
- --
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
-BEGIN PGP SIGNATURE-
Version: GnuPG v2
iQEcBAEBAgAGBQJUpbm7AAoJEFPuqx0v+F+qxFcH/1F6l/1ycTikNdwfsHesZW6Q
TXDO7YfguLDe+wyePEvHdBFW+AwEgGFWTz2EEqfpdu/dHwmWjZh8yTLkJEMfn4fz
jBDm2Kyw+uFzUSXPVCyMUNXgC7yATs6l2AEkX5FSr0BycUVyMIeemChB4Pf5DfQD
B1Lz86xKhM3HpdI/KIDXqqHK88mT/B+7+rUL6lyJ6GRU/168EetD5PzjXoQ67DUw
JaSpCTjf1nUQAL1Nw623QVT4le/vPXLhwpIIAhLmvwO3nn8XWx4WdiCGVm6bGgrV
coWwo8LRB/IMCwoZGh2RhmDy2nOYwj1k9NYLjrx+izhbQUSpwqoLUCZe6vhBTks=
=Ppmw
-END PGP SIGNATURE-
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Drivers: isdn: gigaset: checkpatch cleanup
nts,
all of which actually occur in the driver.
> --- a/drivers/isdn/gigaset/i4l.c +++ b/drivers/isdn/gigaset/i4l.c
> @@ -624,14 +624,14 @@ int gigaset_isdn_regdev(struct cardstate *cs,
> const char *isdnid) { isdn_if *iif;
>
> - iif = kmalloc(sizeof *iif, GFP_KERNEL); + iif =
> kmalloc(sizeof(*iif, GFP_KERNEL)); if (!iif) { pr_err("out of
> memory\n"); return -ENOMEM;
You're calling kmalloc with too few arguments here.
> --- a/drivers/isdn/gigaset/proc.c +++
> b/drivers/isdn/gigaset/proc.c @@ -27,13 +27,18 @@ static ssize_t
> set_cidmode(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count) { struct cardstate *cs =
> dev_get_drvdata(dev); - long int value; - char *end; +long int
> *value; + int result;
>
> - value = simple_strtol(buf, , 0); - while (*end) - if
> (!isspace(*end++)) - return -EINVAL; + result =
> kstrtol(buf, 0,
> ); +if (result == -ERANGE) +/* Overflow error */ +
> dev_err(cs->dev, "Overflow error on conversion from string to\ +
> long\n"); + if (result == -EINVAL) +/* Parsing error */ +
> dev_err(cs->dev, "Parsing error on conversion from string to\ +
> long\n"); if (value < 0 || value > 1) return -EINVAL;
This changes semantics. Your code will not accept the same input as
the original code, and it will emit messages of its own instead of
just returning an error code to the caller as it should.
> --- a/drivers/isdn/gigaset/usb-gigaset.c +++
> b/drivers/isdn/gigaset/usb-gigaset.c
> default: rate = 9600; - dev_err(cs->dev, "unsupported baudrate
> request 0x%x," - " using default of B9600\n", cflag); +
> dev_err(cs->dev, "unsupported baudrate request 0x%x,\ +
> using
> default of B9600\n", cflag);
This makes the message much less readable by inserting a long stretch
of whitespace after the comma.
In sum: NACK.
Regards,
Tilman
- --
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
-BEGIN PGP SIGNATURE-
Version: GnuPG v2
iQEcBAEBAgAGBQJUpZYFAAoJEFPuqx0v+F+qEcsH/1yyu8A8tHPhiW60DzQWhCj7
kxKw7gUS24ATLEEl5jUmrua2xPM0Exg7FknBSYRmNmOEj8j3sl7mQ0dDzDcJgOgI
BaDXV5YqnnqppmYkdT7OMykAuhdt2rk1w4khc2EjyyKrAdGJyB+j3ROgRDtG0wsY
zI/Uz7yKe540cwVWc6VCNQvS7cVEasQZnJzzTGBcPW35RjTYpvWEieJ/yY3tphIe
TQRl+SrKgiwGuzi0p886Vk8Mu4cfHHO5/EXyzpVdMVg6wxwxNs+YeW5xRf/mjzQe
YyygRKUA4VtZTno/rabdA2QvLkdDMHoiUNYa0InmBpAlLVol8OLh5mTit9yozwY=
=uU+S
-END PGP SIGNATURE-
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Drivers: isdn: gigaset: checkpatch cleanup
/drivers/isdn/gigaset/proc.c @@ -27,13 +27,18 @@ static ssize_t
set_cidmode(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count) { struct cardstate *cs =
dev_get_drvdata(dev); - long int value; - char *end; +long int
*value; + int result;
- value = simple_strtol(buf, end, 0); - while (*end) - if
(!isspace(*end++)) - return -EINVAL; + result =
kstrtol(buf, 0,
value); +if (result == -ERANGE) +/* Overflow error */ +
dev_err(cs-dev, Overflow error on conversion from string to\ +
long\n); + if (result == -EINVAL) +/* Parsing error */ +
dev_err(cs-dev, Parsing error on conversion from string to\ +
long\n); if (value 0 || value 1) return -EINVAL;
This changes semantics. Your code will not accept the same input as
the original code, and it will emit messages of its own instead of
just returning an error code to the caller as it should.
--- a/drivers/isdn/gigaset/usb-gigaset.c +++
b/drivers/isdn/gigaset/usb-gigaset.c
default: rate = 9600; - dev_err(cs-dev, unsupported baudrate
request 0x%x, - using default of B9600\n, cflag); +
dev_err(cs-dev, unsupported baudrate request 0x%x,\ +
using
default of B9600\n, cflag);
This makes the message much less readable by inserting a long stretch
of whitespace after the comma.
In sum: NACK.
Regards,
Tilman
- --
Tilman Schmidt E-Mail: til...@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
-BEGIN PGP SIGNATURE-
Version: GnuPG v2
iQEcBAEBAgAGBQJUpZYFAAoJEFPuqx0v+F+qEcsH/1yyu8A8tHPhiW60DzQWhCj7
kxKw7gUS24ATLEEl5jUmrua2xPM0Exg7FknBSYRmNmOEj8j3sl7mQ0dDzDcJgOgI
BaDXV5YqnnqppmYkdT7OMykAuhdt2rk1w4khc2EjyyKrAdGJyB+j3ROgRDtG0wsY
zI/Uz7yKe540cwVWc6VCNQvS7cVEasQZnJzzTGBcPW35RjTYpvWEieJ/yY3tphIe
TQRl+SrKgiwGuzi0p886Vk8Mu4cfHHO5/EXyzpVdMVg6wxwxNs+YeW5xRf/mjzQe
YyygRKUA4VtZTno/rabdA2QvLkdDMHoiUNYa0InmBpAlLVol8OLh5mTit9yozwY=
=uU+S
-END PGP SIGNATURE-
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
