linux-omap.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] video: Remove in_interrupt() usage.
@ 2021-02-08 22:38 Sebastian Andrzej Siewior
  2021-02-08 22:38 ` [PATCH 1/3] video: omap: " Sebastian Andrzej Siewior
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-02-08 22:38 UTC (permalink / raw)
  To: linux-fbdev
  Cc: dri-devel, Russell King, linux-omap, Thomas Gleixner, Greg Kroah-Hartman

Folks,

in the discussion about preempt count consistency across kernel
configurations:

 https://lore.kernel.org/r/20200914204209.256266093@linutronix.de/

it was concluded that the usage of in_interrupt() and related context
checks should be removed from non-core code.

In the long run, usage of 'preemptible, in_*irq etc.' should be banned from
driver code completely.

This series targets the video subsystem. The omap patches are a repost
of [0], the amba-clcd is new after I received no feedback on my analysis
[1].

[0] https://lkml.kernel.org/r/20210127172902.145335-1-bigeasy@linutronix.de
[1] https://lkml.kernel.org/r/20210127174408.ududpwfrbg3dhyxj@linutronix.de

Sebastian



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/3] video: omap: Remove in_interrupt() usage.
  2021-02-08 22:38 [PATCH 0/3] video: Remove in_interrupt() usage Sebastian Andrzej Siewior
@ 2021-02-08 22:38 ` Sebastian Andrzej Siewior
  2021-02-08 22:38 ` [PATCH 2/3] video: omapfb: Remove WARN_ON(in_interrupt()) Sebastian Andrzej Siewior
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 7+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-02-08 22:38 UTC (permalink / raw)
  To: linux-fbdev
  Cc: dri-devel, Russell King, linux-omap, Thomas Gleixner,
	Greg Kroah-Hartman, Ahmed S. Darwish, Sebastian Andrzej Siewior

From: "Ahmed S. Darwish" <a.darwish@linutronix.de>

alloc_req() uses in_interrupt() to detect if it is safe to use down().

The usage of in_interrupt() in drivers is phased out and Linus clearly
requested that code which changes behaviour depending on context should
either be separated or the context be conveyed in an argument passed by the
caller, which usually knows the context.

The semaphore is used as a counting semaphore, initialized with the
number of slots in the request pool minus IRQ_REQ_POOL_SIZE - which are
reserved for the in_interrupt() user to ensure that a request is always
available. The preemptible user will block on the semphore waiting for a
request to become available in case there are no requests available.

Replace in_interrupt() with a `can_sleep' argument to indicate if it is
safe to block on the sempahore.

Cc: linux-omap@vger.kernel.org
Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/video/fbdev/omap/hwa742.c | 42 ++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/video/fbdev/omap/hwa742.c b/drivers/video/fbdev/omap/hwa742.c
index cfe63932f8255..b191bef22d984 100644
--- a/drivers/video/fbdev/omap/hwa742.c
+++ b/drivers/video/fbdev/omap/hwa742.c
@@ -100,6 +100,14 @@ struct {
 	struct hwa742_request	req_pool[REQ_POOL_SIZE];
 	struct list_head	pending_req_list;
 	struct list_head	free_req_list;
+
+	/*
+	 * @req_lock: protect request slots pool and its tracking lists
+	 * @req_sema: counter; slot allocators from task contexts must
+	 *            push it down before acquiring a slot. This
+	 *            guarantees that atomic contexts will always have
+	 *            a minimum of IRQ_REQ_POOL_SIZE slots available.
+	 */
 	struct semaphore	req_sema;
 	spinlock_t		req_lock;
 
@@ -224,13 +232,13 @@ static void disable_tearsync(void)
 	hwa742_write_reg(HWA742_NDP_CTRL, b);
 }
 
-static inline struct hwa742_request *alloc_req(void)
+static inline struct hwa742_request *alloc_req(bool can_sleep)
 {
 	unsigned long flags;
 	struct hwa742_request *req;
 	int req_flags = 0;
 
-	if (!in_interrupt())
+	if (can_sleep)
 		down(&hwa742.req_sema);
 	else
 		req_flags = REQ_FROM_IRQ_POOL;
@@ -399,8 +407,8 @@ static void send_frame_complete(void *data)
 	hwa742.int_ctrl->enable_plane(OMAPFB_PLANE_GFX, 0);
 }
 
-#define ADD_PREQ(_x, _y, _w, _h) do {		\
-	req = alloc_req();			\
+#define ADD_PREQ(_x, _y, _w, _h, can_sleep) do {\
+	req = alloc_req(can_sleep);		\
 	req->handler	= send_frame_handler;	\
 	req->complete	= send_frame_complete;	\
 	req->par.update.x = _x;			\
@@ -413,7 +421,8 @@ static void send_frame_complete(void *data)
 } while(0)
 
 static void create_req_list(struct omapfb_update_window *win,
-			    struct list_head *req_head)
+			    struct list_head *req_head,
+			    bool can_sleep)
 {
 	struct hwa742_request *req;
 	int x = win->x;
@@ -427,7 +436,7 @@ static void create_req_list(struct omapfb_update_window *win,
 	color_mode = win->format & OMAPFB_FORMAT_MASK;
 
 	if (x & 1) {
-		ADD_PREQ(x, y, 1, height);
+		ADD_PREQ(x, y, 1, height, can_sleep);
 		width--;
 		x++;
 		flags &= ~OMAPFB_FORMAT_FLAG_TEARSYNC;
@@ -439,19 +448,19 @@ static void create_req_list(struct omapfb_update_window *win,
 
 		if (xspan * height * 2 > hwa742.max_transmit_size) {
 			yspan = hwa742.max_transmit_size / (xspan * 2);
-			ADD_PREQ(x, ystart, xspan, yspan);
+			ADD_PREQ(x, ystart, xspan, yspan, can_sleep);
 			ystart += yspan;
 			yspan = height - yspan;
 			flags &= ~OMAPFB_FORMAT_FLAG_TEARSYNC;
 		}
 
-		ADD_PREQ(x, ystart, xspan, yspan);
+		ADD_PREQ(x, ystart, xspan, yspan, can_sleep);
 		x += xspan;
 		width -= xspan;
 		flags &= ~OMAPFB_FORMAT_FLAG_TEARSYNC;
 	}
 	if (width)
-		ADD_PREQ(x, y, 1, height);
+		ADD_PREQ(x, y, 1, height, can_sleep);
 }
 
 static void auto_update_complete(void *data)
@@ -461,12 +470,12 @@ static void auto_update_complete(void *data)
 			  jiffies + HWA742_AUTO_UPDATE_TIME);
 }
 
-static void hwa742_update_window_auto(struct timer_list *unused)
+static void __hwa742_update_window_auto(bool can_sleep)
 {
 	LIST_HEAD(req_list);
 	struct hwa742_request *last;
 
-	create_req_list(&hwa742.auto_update_window, &req_list);
+	create_req_list(&hwa742.auto_update_window, &req_list, can_sleep);
 	last = list_entry(req_list.prev, struct hwa742_request, entry);
 
 	last->complete = auto_update_complete;
@@ -475,6 +484,11 @@ static void hwa742_update_window_auto(struct timer_list *unused)
 	submit_req_list(&req_list);
 }
 
+static void hwa742_update_window_auto(struct timer_list *unused)
+{
+	__hwa742_update_window_auto(false);
+}
+
 int hwa742_update_window_async(struct fb_info *fbi,
 				 struct omapfb_update_window *win,
 				 void (*complete_callback)(void *arg),
@@ -497,7 +511,7 @@ int hwa742_update_window_async(struct fb_info *fbi,
 		goto out;
 	}
 
-	create_req_list(win, &req_list);
+	create_req_list(win, &req_list, true);
 	last = list_entry(req_list.prev, struct hwa742_request, entry);
 
 	last->complete = complete_callback;
@@ -544,7 +558,7 @@ static void hwa742_sync(void)
 	struct hwa742_request *req;
 	struct completion comp;
 
-	req = alloc_req();
+	req = alloc_req(true);
 
 	req->handler = sync_handler;
 	req->complete = NULL;
@@ -599,7 +613,7 @@ static int hwa742_set_update_mode(enum omapfb_update_mode mode)
 		omapfb_notify_clients(hwa742.fbdev, OMAPFB_EVENT_READY);
 		break;
 	case OMAPFB_AUTO_UPDATE:
-		hwa742_update_window_auto(0);
+		__hwa742_update_window_auto(true);
 		break;
 	case OMAPFB_UPDATE_DISABLED:
 		break;
-- 
2.30.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/3] video: omapfb: Remove WARN_ON(in_interrupt()).
  2021-02-08 22:38 [PATCH 0/3] video: Remove in_interrupt() usage Sebastian Andrzej Siewior
  2021-02-08 22:38 ` [PATCH 1/3] video: omap: " Sebastian Andrzej Siewior
@ 2021-02-08 22:38 ` Sebastian Andrzej Siewior
  2021-02-08 22:38 ` [PATCH 3/3] video: fbdev: amba-clcd: Always use msleep() for waiting Sebastian Andrzej Siewior
  2021-02-16  8:35 ` [PATCH 0/3] video: Remove in_interrupt() usage Sebastian Andrzej Siewior
  3 siblings, 0 replies; 7+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-02-08 22:38 UTC (permalink / raw)
  To: linux-fbdev
  Cc: dri-devel, Russell King, linux-omap, Thomas Gleixner,
	Greg Kroah-Hartman, Ahmed S. Darwish, Sebastian Andrzej Siewior

From: "Ahmed S. Darwish" <a.darwish@linutronix.de>

dsi_sync_vc() uses in_interrupt() to create a warning if the function is
used in non-preemptible context.

The usage of in_interrupt() in drivers is phased out and Linus clearly
requested that code which changes behaviour depending on context should
either be separated or the context be conveyed in an argument passed by the
caller, which usually knows the context.

The wait_for_completion() function (used in dsi_sync_vc_vp() and
dsi_sync_vc_l4() has already a check if it is invoked from proper
context.

Remove WARN_ON(in_interrupt()) from the driver.

Cc: linux-omap@vger.kernel.org
Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/video/fbdev/omap2/omapfb/dss/dsi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dsi.c b/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
index dc34bb04b865c..df90091de75f8 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
@@ -2373,8 +2373,6 @@ static int dsi_sync_vc(struct platform_device *dsidev, int channel)
 
 	WARN_ON_ONCE(!dsi_bus_is_locked(dsidev));
 
-	WARN_ON(in_interrupt());
-
 	if (!dsi_vc_is_enabled(dsidev, channel))
 		return 0;
 
-- 
2.30.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 3/3] video: fbdev: amba-clcd: Always use msleep() for waiting
  2021-02-08 22:38 [PATCH 0/3] video: Remove in_interrupt() usage Sebastian Andrzej Siewior
  2021-02-08 22:38 ` [PATCH 1/3] video: omap: " Sebastian Andrzej Siewior
  2021-02-08 22:38 ` [PATCH 2/3] video: omapfb: Remove WARN_ON(in_interrupt()) Sebastian Andrzej Siewior
@ 2021-02-08 22:38 ` Sebastian Andrzej Siewior
  2021-02-16  8:35 ` [PATCH 0/3] video: Remove in_interrupt() usage Sebastian Andrzej Siewior
  3 siblings, 0 replies; 7+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-02-08 22:38 UTC (permalink / raw)
  To: linux-fbdev
  Cc: dri-devel, Russell King, linux-omap, Thomas Gleixner,
	Greg Kroah-Hartman, Sebastian Andrzej Siewior,
	Peter Collingbourne

The driver uses in_atomic() to distinguish between mdelay() and msleep().

The usage of in_interrupt() in drivers is phased out and Linus clearly
requested that code which changes behaviour depending on context should
either be separated or the context be conveyed in an argument passed by the
caller, which usually knows the context.

I traced the usage of in_interrupt() back to its initial merge:
    bfe694f833643 ("[ARM] Add ARM AMBA CLCD framebuffer driver.")
    https://git.kernel.org/history/history/c/bfe694f833643

The driver has been removed and added back in the meantime.
I've been looking for the IRQ context as described in the comment and
couldn't find it. The functions calling clcdfb_sleep() also call
conditionally backlight_update_status() which acquires a mutex. If it is
okay to acquire a mutex then it is okay to use msleep() since both
functions must be used in preemptible context.

Replace clcdfb_sleep() with msleep().

Cc: Peter Collingbourne <pcc@google.com>
Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/video/fbdev/amba-clcd.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
index 33595cc4778e9..9ec969e136bfd 100644
--- a/drivers/video/fbdev/amba-clcd.c
+++ b/drivers/video/fbdev/amba-clcd.c
@@ -35,19 +35,6 @@
 /* This is limited to 16 characters when displayed by X startup */
 static const char *clcd_name = "CLCD FB";
 
-/*
- * Unfortunately, the enable/disable functions may be called either from
- * process or IRQ context, and we _need_ to delay.  This is _not_ good.
- */
-static inline void clcdfb_sleep(unsigned int ms)
-{
-	if (in_atomic()) {
-		mdelay(ms);
-	} else {
-		msleep(ms);
-	}
-}
-
 static inline void clcdfb_set_start(struct clcd_fb *fb)
 {
 	unsigned long ustart = fb->fb.fix.smem_start;
@@ -77,7 +64,7 @@ static void clcdfb_disable(struct clcd_fb *fb)
 		val &= ~CNTL_LCDPWR;
 		writel(val, fb->regs + fb->off_cntl);
 
-		clcdfb_sleep(20);
+		msleep(20);
 	}
 	if (val & CNTL_LCDEN) {
 		val &= ~CNTL_LCDEN;
@@ -109,7 +96,7 @@ static void clcdfb_enable(struct clcd_fb *fb, u32 cntl)
 	cntl |= CNTL_LCDEN;
 	writel(cntl, fb->regs + fb->off_cntl);
 
-	clcdfb_sleep(20);
+	msleep(20);
 
 	/*
 	 * and now apply power.
-- 
2.30.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/3] video: Remove in_interrupt() usage.
  2021-02-08 22:38 [PATCH 0/3] video: Remove in_interrupt() usage Sebastian Andrzej Siewior
                   ` (2 preceding siblings ...)
  2021-02-08 22:38 ` [PATCH 3/3] video: fbdev: amba-clcd: Always use msleep() for waiting Sebastian Andrzej Siewior
@ 2021-02-16  8:35 ` Sebastian Andrzej Siewior
  2021-02-16  8:43   ` Greg Kroah-Hartman
  3 siblings, 1 reply; 7+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-02-16  8:35 UTC (permalink / raw)
  To: linux-fbdev
  Cc: Greg Kroah-Hartman, Thomas Gleixner, linux-omap, Russell King, dri-devel

On 2021-02-08 23:38:07 [+0100], To linux-fbdev@vger.kernel.org wrote:
> Folks,
> 
> in the discussion about preempt count consistency across kernel
> configurations:
> 
>  https://lore.kernel.org/r/20200914204209.256266093@linutronix.de/
> 
> it was concluded that the usage of in_interrupt() and related context
> checks should be removed from non-core code.
> 
> In the long run, usage of 'preemptible, in_*irq etc.' should be banned from
> driver code completely.
> 
> This series targets the video subsystem. The omap patches are a repost
> of [0], the amba-clcd is new after I received no feedback on my analysis
> [1].
> 
> [0] https://lkml.kernel.org/r/20210127172902.145335-1-bigeasy@linutronix.de
> [1] https://lkml.kernel.org/r/20210127174408.ududpwfrbg3dhyxj@linutronix.de

Could someone please apply the series? Video seems unmaintained.

Sebastian

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/3] video: Remove in_interrupt() usage.
  2021-02-16  8:35 ` [PATCH 0/3] video: Remove in_interrupt() usage Sebastian Andrzej Siewior
@ 2021-02-16  8:43   ` Greg Kroah-Hartman
  2021-02-16  9:09     ` Daniel Vetter
  0 siblings, 1 reply; 7+ messages in thread
From: Greg Kroah-Hartman @ 2021-02-16  8:43 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-fbdev, Thomas Gleixner, linux-omap, Russell King, dri-devel

On Tue, Feb 16, 2021 at 09:35:00AM +0100, Sebastian Andrzej Siewior wrote:
> On 2021-02-08 23:38:07 [+0100], To linux-fbdev@vger.kernel.org wrote:
> > Folks,
> > 
> > in the discussion about preempt count consistency across kernel
> > configurations:
> > 
> >  https://lore.kernel.org/r/20200914204209.256266093@linutronix.de/
> > 
> > it was concluded that the usage of in_interrupt() and related context
> > checks should be removed from non-core code.
> > 
> > In the long run, usage of 'preemptible, in_*irq etc.' should be banned from
> > driver code completely.
> > 
> > This series targets the video subsystem. The omap patches are a repost
> > of [0], the amba-clcd is new after I received no feedback on my analysis
> > [1].
> > 
> > [0] https://lkml.kernel.org/r/20210127172902.145335-1-bigeasy@linutronix.de
> > [1] https://lkml.kernel.org/r/20210127174408.ududpwfrbg3dhyxj@linutronix.de
> 
> Could someone please apply the series? Video seems unmaintained.

It's the merge window, no one can apply the series...

Please resend once 5.12-rc1 is out.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/3] video: Remove in_interrupt() usage.
  2021-02-16  8:43   ` Greg Kroah-Hartman
@ 2021-02-16  9:09     ` Daniel Vetter
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2021-02-16  9:09 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Sebastian Andrzej Siewior, linux-fbdev, linux-omap,
	Thomas Gleixner, dri-devel, Russell King

On Tue, Feb 16, 2021 at 09:43:02AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 16, 2021 at 09:35:00AM +0100, Sebastian Andrzej Siewior wrote:
> > On 2021-02-08 23:38:07 [+0100], To linux-fbdev@vger.kernel.org wrote:
> > > Folks,
> > > 
> > > in the discussion about preempt count consistency across kernel
> > > configurations:
> > > 
> > >  https://lore.kernel.org/r/20200914204209.256266093@linutronix.de/
> > > 
> > > it was concluded that the usage of in_interrupt() and related context
> > > checks should be removed from non-core code.
> > > 
> > > In the long run, usage of 'preemptible, in_*irq etc.' should be banned from
> > > driver code completely.
> > > 
> > > This series targets the video subsystem. The omap patches are a repost
> > > of [0], the amba-clcd is new after I received no feedback on my analysis
> > > [1].
> > > 
> > > [0] https://lkml.kernel.org/r/20210127172902.145335-1-bigeasy@linutronix.de
> > > [1] https://lkml.kernel.org/r/20210127174408.ududpwfrbg3dhyxj@linutronix.de
> > 
> > Could someone please apply the series? Video seems unmaintained.
> 
> It's the merge window, no one can apply the series...
> 
> Please resend once 5.12-rc1 is out.

drm trees are always open, to avoid the merge window blackout lol :-)

Reason I didn't merge anything is that I'm intentionally letting fbdev
hang in there, in the hopes someone picks up review&patch apply duties. It
already worked a few times but then people move on again ...

Anyway patches queued up in drm-misc-next for 5.13.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-02-16  9:10 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-08 22:38 [PATCH 0/3] video: Remove in_interrupt() usage Sebastian Andrzej Siewior
2021-02-08 22:38 ` [PATCH 1/3] video: omap: " Sebastian Andrzej Siewior
2021-02-08 22:38 ` [PATCH 2/3] video: omapfb: Remove WARN_ON(in_interrupt()) Sebastian Andrzej Siewior
2021-02-08 22:38 ` [PATCH 3/3] video: fbdev: amba-clcd: Always use msleep() for waiting Sebastian Andrzej Siewior
2021-02-16  8:35 ` [PATCH 0/3] video: Remove in_interrupt() usage Sebastian Andrzej Siewior
2021-02-16  8:43   ` Greg Kroah-Hartman
2021-02-16  9:09     ` Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).