Re: [PATCH 3/3] serial: Check result of console_unregister()

2018-04-12 Thread Andrey Smirnov 
On Wed, Apr 11, 2018 at 1:34 AM, Sascha Hauer  wrote:
> Hi Andrey,
>
> On Mon, Apr 09, 2018 at 09:00:52AM -0700, Andrey Smirnov wrote:
>> On Tue, Apr 3, 2018 at 12:04 AM, Sascha Hauer  wrote:
>> > On Mon, Mar 26, 2018 at 06:09:15AM -0700, Andrey Smirnov wrote:
>> >> In order to allow 'serdev' devices to prevent parent console device
>> >> removal and correspondign memory deallocation add code to all serial
>> >> driver to check result of console_unregister() and bail out early if
>> >> it is unsuccessful.
>> >>
>> >> One example of a use-case for this would be a reset handler relying on
>> >> a serdev device for transport. Without this patch underlying console
>> >> device would be removed and de-allocated before reset handler is even
>> >> run thus leading to unpredictable behaviour and crashes.
>> >
>> > Can't we make this sure at driver core level?
>>
>> I need to be able to prevent serial driver's "remove" function from
>> ever executing to prevent any de-initialization/memory freeing from
>> happening. The simplest way to solve this in driver core that comes to
>> my mind is implementing simple reference counting API that children
>> could use to force driver core to bail out on removing parents if they
>> are still in use. Does that sound like a reasonable idea?
>>
>> > So if a device decides not
>> > to return -EBUSY in the remove callback then the parent devices won't be
>> > removed?
>>
>> Remove callback currently returns void, we could change it to return
>> int and use it to implement a sort of implicit refcounting, but doing
>> so would result in quite a bit of code churn since all of the current
>> drivers would have to be converted to return int in their .remove
>> callbacks. Would you rather I do this or explicit refcounting?
>
> Normally it helps looking at the Linux kernel to see how a problem is
> solved there. Not so this time it seems. Linux distinguishes between
> "remove" and "shutdown". "shutdown" is what we want during barebox
> shutdown. I found a Linux driver that is similar to your situation: It
> registers a restart_handler while being a i2c device itself. There seems
> to be no way to prevent a device from being shutdown, it's only that the
> i2c bus drivers simply do not implement it.
>
> Where do we go from here? I think reference counting is a bit over the
> top.
>
> At the moment I would opt for a *very* simple solution: Let's drop the
> call to console_unregister() and the freeing of resources entirely as
> it gives us nothing. The only console driver I can see where removing
> is valid is drivers/usb/gadget/u_serial.c and you won't use this for
> restarting a SoC ;)
>

OK, makes sense and works for me. Will do in v2.

Thanks,
Andrey Smirnov

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] serial: Check result of console_unregister()

2018-04-11 Thread Sascha Hauer 
Hi Andrey,

On Mon, Apr 09, 2018 at 09:00:52AM -0700, Andrey Smirnov wrote:
> On Tue, Apr 3, 2018 at 12:04 AM, Sascha Hauer  wrote:
> > On Mon, Mar 26, 2018 at 06:09:15AM -0700, Andrey Smirnov wrote:
> >> In order to allow 'serdev' devices to prevent parent console device
> >> removal and correspondign memory deallocation add code to all serial
> >> driver to check result of console_unregister() and bail out early if
> >> it is unsuccessful.
> >>
> >> One example of a use-case for this would be a reset handler relying on
> >> a serdev device for transport. Without this patch underlying console
> >> device would be removed and de-allocated before reset handler is even
> >> run thus leading to unpredictable behaviour and crashes.
> >
> > Can't we make this sure at driver core level?
> 
> I need to be able to prevent serial driver's "remove" function from
> ever executing to prevent any de-initialization/memory freeing from
> happening. The simplest way to solve this in driver core that comes to
> my mind is implementing simple reference counting API that children
> could use to force driver core to bail out on removing parents if they
> are still in use. Does that sound like a reasonable idea?
> 
> > So if a device decides not
> > to return -EBUSY in the remove callback then the parent devices won't be
> > removed?
> 
> Remove callback currently returns void, we could change it to return
> int and use it to implement a sort of implicit refcounting, but doing
> so would result in quite a bit of code churn since all of the current
> drivers would have to be converted to return int in their .remove
> callbacks. Would you rather I do this or explicit refcounting?

Normally it helps looking at the Linux kernel to see how a problem is
solved there. Not so this time it seems. Linux distinguishes between
"remove" and "shutdown". "shutdown" is what we want during barebox
shutdown. I found a Linux driver that is similar to your situation: It
registers a restart_handler while being a i2c device itself. There seems
to be no way to prevent a device from being shutdown, it's only that the
i2c bus drivers simply do not implement it.

Where do we go from here? I think reference counting is a bit over the
top.

At the moment I would opt for a *very* simple solution: Let's drop the
call to console_unregister() and the freeing of resources entirely as
it gives us nothing. The only console driver I can see where removing
is valid is drivers/usb/gadget/u_serial.c and you won't use this for
restarting a SoC ;)

Sascha

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] serial: Check result of console_unregister()

2018-04-09 Thread Andrey Smirnov 
On Tue, Apr 3, 2018 at 12:04 AM, Sascha Hauer  wrote:
> On Mon, Mar 26, 2018 at 06:09:15AM -0700, Andrey Smirnov wrote:
>> In order to allow 'serdev' devices to prevent parent console device
>> removal and correspondign memory deallocation add code to all serial
>> driver to check result of console_unregister() and bail out early if
>> it is unsuccessful.
>>
>> One example of a use-case for this would be a reset handler relying on
>> a serdev device for transport. Without this patch underlying console
>> device would be removed and de-allocated before reset handler is even
>> run thus leading to unpredictable behaviour and crashes.
>
> Can't we make this sure at driver core level?

I need to be able to prevent serial driver's "remove" function from
ever executing to prevent any de-initialization/memory freeing from
happening. The simplest way to solve this in driver core that comes to
my mind is implementing simple reference counting API that children
could use to force driver core to bail out on removing parents if they
are still in use. Does that sound like a reasonable idea?

> So if a device decides not
> to return -EBUSY in the remove callback then the parent devices won't be
> removed?

Remove callback currently returns void, we could change it to return
int and use it to implement a sort of implicit refcounting, but doing
so would result in quite a bit of code churn since all of the current
drivers would have to be converted to return int in their .remove
callbacks. Would you rather I do this or explicit refcounting?

Thanks,
Andrey Smirnov

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] serial: Check result of console_unregister()

2018-04-03 Thread Sascha Hauer 
On Mon, Mar 26, 2018 at 06:09:15AM -0700, Andrey Smirnov wrote:
> In order to allow 'serdev' devices to prevent parent console device
> removal and correspondign memory deallocation add code to all serial
> driver to check result of console_unregister() and bail out early if
> it is unsuccessful.
> 
> One example of a use-case for this would be a reset handler relying on
> a serdev device for transport. Without this patch underlying console
> device would be removed and de-allocated before reset handler is even
> run thus leading to unpredictable behaviour and crashes.

Can't we make this sure at driver core level? So if a device decides not
to return -EBUSY in the remove callback then the parent devices won't be
removed?

Sascha


-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/3] serial: Check result of console_unregister()

2018-03-26 Thread Andrey Smirnov 
In order to allow 'serdev' devices to prevent parent console device
removal and correspondign memory deallocation add code to all serial
driver to check result of console_unregister() and bail out early if
it is unsuccessful.

One example of a use-case for this would be a reset handler relying on
a serdev device for transport. Without this patch underlying console
device would be removed and de-allocated before reset handler is even
run thus leading to unpredictable behaviour and crashes.

Signed-off-by: Andrey Smirnov 
---
 drivers/serial/serial_auart.c| 6 +-
 drivers/serial/serial_cadence.c  | 6 +-
 drivers/serial/serial_clps711x.c | 6 +-
 drivers/serial/serial_imx.c  | 6 +-
 drivers/serial/serial_lpuart.c   | 6 +-
 drivers/serial/serial_pxa.c  | 6 +-
 drivers/serial/serial_s3c.c  | 6 +-
 drivers/serial/stm-serial.c  | 6 +-
 include/console.h| 2 +-
 9 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/drivers/serial/serial_auart.c b/drivers/serial/serial_auart.c
index c3b9a1995..9bd0d991e 100644
--- a/drivers/serial/serial_auart.c
+++ b/drivers/serial/serial_auart.c
@@ -224,9 +224,13 @@ static int auart_serial_probe(struct device_d *dev)
 static void auart_serial_remove(struct device_d *dev)
 {
struct auart_priv *priv = dev->priv;
+   int ret;
 
auart_serial_flush(>cdev);
-   console_unregister(>cdev);
+   ret = console_unregister(>cdev);
+   if (ret < 0)
+   return;
+
free(priv);
 }
 
diff --git a/drivers/serial/serial_cadence.c b/drivers/serial/serial_cadence.c
index 36dfa2084..f43d98172 100644
--- a/drivers/serial/serial_cadence.c
+++ b/drivers/serial/serial_cadence.c
@@ -270,8 +270,12 @@ err_free:
 static void cadence_serial_remove(struct device_d *dev)
 {
struct cadence_serial_priv *priv = dev->priv;
+   int ret;
+
+   ret = console_unregister(>cdev);
+   if (ret < 0)
+   return;
 
-   console_unregister(>cdev);
free(priv);
 }
 
diff --git a/drivers/serial/serial_clps711x.c b/drivers/serial/serial_clps711x.c
index ad14373ac..24ae3fdd3 100644
--- a/drivers/serial/serial_clps711x.c
+++ b/drivers/serial/serial_clps711x.c
@@ -184,9 +184,13 @@ out_err:
 static void clps711x_remove(struct device_d *dev)
 {
struct clps711x_uart *s = dev->priv;
+   int ret;
 
clps711x_flush(>cdev);
-   console_unregister(>cdev);
+   ret = console_unregister(>cdev);
+   if (ret < 0)
+   return;
+
free(s);
 }
 
diff --git a/drivers/serial/serial_imx.c b/drivers/serial/serial_imx.c
index e8c3836a6..fb865f554 100644
--- a/drivers/serial/serial_imx.c
+++ b/drivers/serial/serial_imx.c
@@ -274,9 +274,13 @@ err_free:
 static void imx_serial_remove(struct device_d *dev)
 {
struct imx_serial_priv *priv = dev->priv;
+   int ret;
 
imx_serial_flush(>cdev);
-   console_unregister(>cdev);
+   ret = console_unregister(>cdev);
+   if (ret < 0)
+   return;
+
free(priv);
 }
 
diff --git a/drivers/serial/serial_lpuart.c b/drivers/serial/serial_lpuart.c
index 52fb6d39c..42fab8a56 100644
--- a/drivers/serial/serial_lpuart.c
+++ b/drivers/serial/serial_lpuart.c
@@ -194,9 +194,13 @@ err_free:
 static void lpuart_serial_remove(struct device_d *dev)
 {
struct lpuart *lpuart = dev->priv;
+   int ret;
 
lpuart_serial_flush(>cdev);
-   console_unregister(>cdev);
+   ret = console_unregister(>cdev);
+   if (ret < 0)
+   return;
+
release_region(lpuart->io);
clk_put(lpuart->clk);
 
diff --git a/drivers/serial/serial_pxa.c b/drivers/serial/serial_pxa.c
index 1a4d7b430..146f19cf3 100644
--- a/drivers/serial/serial_pxa.c
+++ b/drivers/serial/serial_pxa.c
@@ -188,8 +188,12 @@ static int pxa_serial_probe(struct device_d *dev)
 static void pxa_serial_remove(struct device_d *dev)
 {
struct pxa_serial_priv *priv = dev->priv;
+   int ret;
+
+   ret = console_unregister(>cdev);
+   if (ret < 0)
+   return;
 
-   console_unregister(>cdev);
free(priv);
 }
 
diff --git a/drivers/serial/serial_s3c.c b/drivers/serial/serial_s3c.c
index 0a6e22d97..8c6443acd 100644
--- a/drivers/serial/serial_s3c.c
+++ b/drivers/serial/serial_s3c.c
@@ -205,9 +205,13 @@ static int s3c_serial_probe(struct device_d *dev)
 static void s3c_serial_remove(struct device_d *dev)
 {
struct s3c_uart *priv= dev->priv;
+   int ret;
 
s3c_serial_flush(>cdev);
-   console_unregister(>cdev);
+   ret = console_unregister(>cdev);
+   if (ret < 0)
+   return;
+
free(priv);
 }
 
diff --git a/drivers/serial/stm-serial.c b/drivers/serial/stm-serial.c
index 83328f455..3edcdd7e8 100644
--- a/drivers/serial/stm-serial.c
+++ b/drivers/serial/stm-serial.c
@@ -185,9 +185,13 @@ static int stm_serial_probe(struct device_d *dev)
 static void