[PATCH v2 2/9] dmapool: cleanup error messages

[PATCH v2 2/9] dmapool: cleanup error messages

[Tony Battersby]

Remove code duplication in error messages.  It is now safe to pas a NULL
dev to dev_err(), so the checks to avoid doing so are no longer
necessary.

Example:

Error message with dev != NULL:
  mpt3sas 0000:02:00.0: dma_pool_destroy chain pool, (____ptrval____) busy

Same error message with dev == NULL before patch:
  dma_pool_destroy chain pool, (____ptrval____) busy

Same error message with dev == NULL after patch:
  (NULL device *): dma_pool_destroy chain pool, (____ptrval____) busy

Signed-off-by: Tony Battersby <tonyb-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.org>
---
--- linux/mm/dmapool.c.orig	2018-08-02 09:54:25.000000000 -0400
+++ linux/mm/dmapool.c	2018-08-02 09:57:58.000000000 -0400
@@ -289,13 +289,9 @@ void dma_pool_destroy(struct dma_pool *p
 		page = list_entry(pool->page_list.next,
 				  struct dma_page, page_list);
 		if (is_page_busy(page)) {
-			if (pool->dev)
-				dev_err(pool->dev,
-					"dma_pool_destroy %s, %p busy\n",
-					pool->name, page->vaddr);
-			else
-				pr_err("dma_pool_destroy %s, %p busy\n",
-				       pool->name, page->vaddr);
+			dev_err(pool->dev,
+				"dma_pool_destroy %s, %p busy\n",
+				pool->name, page->vaddr);
 			/* leak the still-in-use consistent memory */
 			list_del(&page->page_list);
 			kfree(page);
@@ -357,13 +353,9 @@ void *dma_pool_alloc(struct dma_pool *po
 		for (i = sizeof(page->offset); i < pool->size; i++) {
 			if (data[i] == POOL_POISON_FREED)
 				continue;
-			if (pool->dev)
-				dev_err(pool->dev,
-					"dma_pool_alloc %s, %p (corrupted)\n",
-					pool->name, retval);
-			else
-				pr_err("dma_pool_alloc %s, %p (corrupted)\n",
-					pool->name, retval);
+			dev_err(pool->dev,
+				"dma_pool_alloc %s, %p (corrupted)\n",
+				pool->name, retval);
 
 			/*
 			 * Dump the first 4 bytes even if they are not
@@ -418,13 +410,9 @@ void dma_pool_free(struct dma_pool *pool
 	page = pool_find_page(pool, dma);
 	if (!page) {
 		spin_unlock_irqrestore(&pool->lock, flags);
-		if (pool->dev)
-			dev_err(pool->dev,
-				"dma_pool_free %s, %p/%lx (bad dma)\n",
-				pool->name, vaddr, (unsigned long)dma);
-		else
-			pr_err("dma_pool_free %s, %p/%lx (bad dma)\n",
-			       pool->name, vaddr, (unsigned long)dma);
+		dev_err(pool->dev,
+			"dma_pool_free %s, %p/%lx (bad dma)\n",
+			pool->name, vaddr, (unsigned long)dma);
 		return;
 	}
 
@@ -432,13 +420,9 @@ void dma_pool_free(struct dma_pool *pool
 #ifdef	DMAPOOL_DEBUG
 	if ((dma - page->dma) != offset) {
 		spin_unlock_irqrestore(&pool->lock, flags);
-		if (pool->dev)
-			dev_err(pool->dev,
-				"dma_pool_free %s, %p (bad vaddr)/%pad\n",
-				pool->name, vaddr, &dma);
-		else
-			pr_err("dma_pool_free %s, %p (bad vaddr)/%pad\n",
-			       pool->name, vaddr, &dma);
+		dev_err(pool->dev,
+			"dma_pool_free %s, %p (bad vaddr)/%pad\n",
+			pool->name, vaddr, &dma);
 		return;
 	}
 	{
@@ -449,12 +433,9 @@ void dma_pool_free(struct dma_pool *pool
 				continue;
 			}
 			spin_unlock_irqrestore(&pool->lock, flags);
-			if (pool->dev)
-				dev_err(pool->dev, "dma_pool_free %s, dma %pad already free\n",
-					pool->name, &dma);
-			else
-				pr_err("dma_pool_free %s, dma %pad already free\n",
-				       pool->name, &dma);
+			dev_err(pool->dev,
+				"dma_pool_free %s, dma %pad already free\n",
+				pool->name, &dma);
 			return;
 		}
 	}

[PATCH v2 2/9] dmapool: cleanup error messages

[Tony Battersby]

Remove code duplication in error messages.  It is now safe to pas a NULL
dev to dev_err(), so the checks to avoid doing so are no longer
necessary.

Example:

Error message with dev != NULL:
  mpt3sas 0000:02:00.0: dma_pool_destroy chain pool, (____ptrval____) busy

Same error message with dev == NULL before patch:
  dma_pool_destroy chain pool, (____ptrval____) busy

Same error message with dev == NULL after patch:
  (NULL device *): dma_pool_destroy chain pool, (____ptrval____) busy

Signed-off-by: Tony Battersby <tonyb@cybernetics.com>
---
--- linux/mm/dmapool.c.orig	2018-08-02 09:54:25.000000000 -0400
+++ linux/mm/dmapool.c	2018-08-02 09:57:58.000000000 -0400
@@ -289,13 +289,9 @@ void dma_pool_destroy(struct dma_pool *p
 		page = list_entry(pool->page_list.next,
 				  struct dma_page, page_list);
 		if (is_page_busy(page)) {
-			if (pool->dev)
-				dev_err(pool->dev,
-					"dma_pool_destroy %s, %p busy\n",
-					pool->name, page->vaddr);
-			else
-				pr_err("dma_pool_destroy %s, %p busy\n",
-				       pool->name, page->vaddr);
+			dev_err(pool->dev,
+				"dma_pool_destroy %s, %p busy\n",
+				pool->name, page->vaddr);
 			/* leak the still-in-use consistent memory */
 			list_del(&page->page_list);
 			kfree(page);
@@ -357,13 +353,9 @@ void *dma_pool_alloc(struct dma_pool *po
 		for (i = sizeof(page->offset); i < pool->size; i++) {
 			if (data[i] == POOL_POISON_FREED)
 				continue;
-			if (pool->dev)
-				dev_err(pool->dev,
-					"dma_pool_alloc %s, %p (corrupted)\n",
-					pool->name, retval);
-			else
-				pr_err("dma_pool_alloc %s, %p (corrupted)\n",
-					pool->name, retval);
+			dev_err(pool->dev,
+				"dma_pool_alloc %s, %p (corrupted)\n",
+				pool->name, retval);
 
 			/*
 			 * Dump the first 4 bytes even if they are not
@@ -418,13 +410,9 @@ void dma_pool_free(struct dma_pool *pool
 	page = pool_find_page(pool, dma);
 	if (!page) {
 		spin_unlock_irqrestore(&pool->lock, flags);
-		if (pool->dev)
-			dev_err(pool->dev,
-				"dma_pool_free %s, %p/%lx (bad dma)\n",
-				pool->name, vaddr, (unsigned long)dma);
-		else
-			pr_err("dma_pool_free %s, %p/%lx (bad dma)\n",
-			       pool->name, vaddr, (unsigned long)dma);
+		dev_err(pool->dev,
+			"dma_pool_free %s, %p/%lx (bad dma)\n",
+			pool->name, vaddr, (unsigned long)dma);
 		return;
 	}
 
@@ -432,13 +420,9 @@ void dma_pool_free(struct dma_pool *pool
 #ifdef	DMAPOOL_DEBUG
 	if ((dma - page->dma) != offset) {
 		spin_unlock_irqrestore(&pool->lock, flags);
-		if (pool->dev)
-			dev_err(pool->dev,
-				"dma_pool_free %s, %p (bad vaddr)/%pad\n",
-				pool->name, vaddr, &dma);
-		else
-			pr_err("dma_pool_free %s, %p (bad vaddr)/%pad\n",
-			       pool->name, vaddr, &dma);
+		dev_err(pool->dev,
+			"dma_pool_free %s, %p (bad vaddr)/%pad\n",
+			pool->name, vaddr, &dma);
 		return;
 	}
 	{
@@ -449,12 +433,9 @@ void dma_pool_free(struct dma_pool *pool
 				continue;
 			}
 			spin_unlock_irqrestore(&pool->lock, flags);
-			if (pool->dev)
-				dev_err(pool->dev, "dma_pool_free %s, dma %pad already free\n",
-					pool->name, &dma);
-			else
-				pr_err("dma_pool_free %s, dma %pad already free\n",
-				       pool->name, &dma);
+			dev_err(pool->dev,
+				"dma_pool_free %s, dma %pad already free\n",
+				pool->name, &dma);
 			return;
 		}
 	}

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Andy Shevchenko]

On Thu, Aug 2, 2018 at 10:57 PM, Tony Battersby <tonyb-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.org> wrote:
> Remove code duplication in error messages.  It is now safe to pas a NULL
> dev to dev_err(), so the checks to avoid doing so are no longer
> necessary.
>
> Example:
>
> Error message with dev != NULL:
>   mpt3sas 0000:02:00.0: dma_pool_destroy chain pool, (____ptrval____) busy
>
> Same error message with dev == NULL before patch:
>   dma_pool_destroy chain pool, (____ptrval____) busy
>
> Same error message with dev == NULL after patch:
>   (NULL device *): dma_pool_destroy chain pool, (____ptrval____) busy

Have you checked a history of this?

I'm pretty sure this was created in an order to avoid bad looking (and
in some cases frightening) "NULL device *" part.

If it it's the case, I would rather leave it as is, and even not the
case, I'm slightly more bent to the current state.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Andy Shevchenko]

On Thu, Aug 2, 2018 at 10:57 PM, Tony Battersby <tonyb@cybernetics.com> wrote:
> Remove code duplication in error messages.  It is now safe to pas a NULL
> dev to dev_err(), so the checks to avoid doing so are no longer
> necessary.
>
> Example:
>
> Error message with dev != NULL:
>   mpt3sas 0000:02:00.0: dma_pool_destroy chain pool, (____ptrval____) busy
>
> Same error message with dev == NULL before patch:
>   dma_pool_destroy chain pool, (____ptrval____) busy
>
> Same error message with dev == NULL after patch:
>   (NULL device *): dma_pool_destroy chain pool, (____ptrval____) busy

Have you checked a history of this?

I'm pretty sure this was created in an order to avoid bad looking (and
in some cases frightening) "NULL device *" part.

If it it's the case, I would rather leave it as is, and even not the
case, I'm slightly more bent to the current state.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Tony Battersby]

On 08/03/2018 04:56 AM, Andy Shevchenko wrote:
> On Thu, Aug 2, 2018 at 10:57 PM, Tony Battersby <tonyb@cybernetics.com> wrote:
>> Remove code duplication in error messages.  It is now safe to pas a NULL
>> dev to dev_err(), so the checks to avoid doing so are no longer
>> necessary.
>>
>> Example:
>>
>> Error message with dev != NULL:
>>   mpt3sas 0000:02:00.0: dma_pool_destroy chain pool, (____ptrval____) busy
>>
>> Same error message with dev == NULL before patch:
>>   dma_pool_destroy chain pool, (____ptrval____) busy
>>
>> Same error message with dev == NULL after patch:
>>   (NULL device *): dma_pool_destroy chain pool, (____ptrval____) busy
> Have you checked a history of this?
>
> I'm pretty sure this was created in an order to avoid bad looking (and
> in some cases frightening) "NULL device *" part.
>
> If it it's the case, I would rather leave it as is, and even not the
> case, I'm slightly more bent to the current state.
>
I did.  "drivers/base/dmapool.c", later moved to "mm/dmapool.c", was
added in linux-2.6.3, for which dev_err() did not work will a NULL dev,
so the check was necessary back then.  I agree that the (NULL device *):
bit is ugly, but these messages should be printed only after a kernel
bug, so it is not like they will be making a regular appearance in
dmesg.  Considering that, I think that it is better to keep it simple.

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Tony Battersby]

On 08/03/2018 04:56 AM, Andy Shevchenko wrote:
> On Thu, Aug 2, 2018 at 10:57 PM, Tony Battersby <tonyb@cybernetics.com> wrote:
>> Remove code duplication in error messages.  It is now safe to pas a NULL
>> dev to dev_err(), so the checks to avoid doing so are no longer
>> necessary.
>>
>> Example:
>>
>> Error message with dev != NULL:
>>   mpt3sas 0000:02:00.0: dma_pool_destroy chain pool, (____ptrval____) busy
>>
>> Same error message with dev == NULL before patch:
>>   dma_pool_destroy chain pool, (____ptrval____) busy
>>
>> Same error message with dev == NULL after patch:
>>   (NULL device *): dma_pool_destroy chain pool, (____ptrval____) busy
> Have you checked a history of this?
>
> I'm pretty sure this was created in an order to avoid bad looking (and
> in some cases frightening) "NULL device *" part.
>
> If it it's the case, I would rather leave it as is, and even not the
> case, I'm slightly more bent to the current state.
>
I did.A  "drivers/base/dmapool.c", later moved to "mm/dmapool.c", was
added in linux-2.6.3, for which dev_err() did not work will a NULL dev,
so the check was necessary back then.A  I agree that the (NULL device *):
bit is ugly, but these messages should be printed only after a kernel
bug, so it is not like they will be making a regular appearance in
dmesg.A  Considering that, I think that it is better to keep it simple.

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Tony Battersby]

On 08/03/2018 09:41 AM, Tony Battersby wrote:
> On 08/03/2018 04:56 AM, Andy Shevchenko wrote:
>> On Thu, Aug 2, 2018 at 10:57 PM, Tony Battersby <tonyb@cybernetics.com> wrote:
>>> Remove code duplication in error messages.  It is now safe to pas a NULL
>>> dev to dev_err(), so the checks to avoid doing so are no longer
>>> necessary.
>>>
>>> Example:
>>>
>>> Error message with dev != NULL:
>>>   mpt3sas 0000:02:00.0: dma_pool_destroy chain pool, (____ptrval____) busy
>>>
>>> Same error message with dev == NULL before patch:
>>>   dma_pool_destroy chain pool, (____ptrval____) busy
>>>
>>> Same error message with dev == NULL after patch:
>>>   (NULL device *): dma_pool_destroy chain pool, (____ptrval____) busy
>> Have you checked a history of this?
>>
>> I'm pretty sure this was created in an order to avoid bad looking (and
>> in some cases frightening) "NULL device *" part.
>>
>> If it it's the case, I would rather leave it as is, and even not the
>> case, I'm slightly more bent to the current state.
>>
> I did.  "drivers/base/dmapool.c", later moved to "mm/dmapool.c", was
> added in linux-2.6.3, for which dev_err() did not work will a NULL dev,
> so the check was necessary back then.  I agree that the (NULL device *):
> bit is ugly, but these messages should be printed only after a kernel
> bug, so it is not like they will be making a regular appearance in
> dmesg.  Considering that, I think that it is better to keep it simple.
>

My original unsubmitted patch used the following:

+#define pool_err(pool, fmt, args...) \
+	do { \
+		if ((pool)->dev) \
+			dev_err((pool)->dev, fmt, args); \
+		else \
+			pr_err(fmt, args); \
+	} while (0)

But then I decided to simplify it to just use dev_err().  I still have
the old version.  When I submit v3 of the patchset, which would you prefer?
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Tony Battersby]

On 08/03/2018 09:41 AM, Tony Battersby wrote:
> On 08/03/2018 04:56 AM, Andy Shevchenko wrote:
>> On Thu, Aug 2, 2018 at 10:57 PM, Tony Battersby <tonyb@cybernetics.com> wrote:
>>> Remove code duplication in error messages.  It is now safe to pas a NULL
>>> dev to dev_err(), so the checks to avoid doing so are no longer
>>> necessary.
>>>
>>> Example:
>>>
>>> Error message with dev != NULL:
>>>   mpt3sas 0000:02:00.0: dma_pool_destroy chain pool, (____ptrval____) busy
>>>
>>> Same error message with dev == NULL before patch:
>>>   dma_pool_destroy chain pool, (____ptrval____) busy
>>>
>>> Same error message with dev == NULL after patch:
>>>   (NULL device *): dma_pool_destroy chain pool, (____ptrval____) busy
>> Have you checked a history of this?
>>
>> I'm pretty sure this was created in an order to avoid bad looking (and
>> in some cases frightening) "NULL device *" part.
>>
>> If it it's the case, I would rather leave it as is, and even not the
>> case, I'm slightly more bent to the current state.
>>
> I did.A  "drivers/base/dmapool.c", later moved to "mm/dmapool.c", was
> added in linux-2.6.3, for which dev_err() did not work will a NULL dev,
> so the check was necessary back then.A  I agree that the (NULL device *):
> bit is ugly, but these messages should be printed only after a kernel
> bug, so it is not like they will be making a regular appearance in
> dmesg.A  Considering that, I think that it is better to keep it simple.
>

My original unsubmitted patch used the following:

+#define pool_err(pool, fmt, args...) \
+	do { \
+		if ((pool)->dev) \
+			dev_err((pool)->dev, fmt, args); \
+		else \
+			pr_err(fmt, args); \
+	} while (0)

But then I decided to simplify it to just use dev_err().A  I still have
the old version.A  When I submit v3 of the patchset, which would you prefer?

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Andy Shevchenko]

On Fri, Aug 3, 2018 at 6:17 PM, Tony Battersby <tonyb-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.org> wrote:
> On 08/03/2018 09:41 AM, Tony Battersby wrote:
>> On 08/03/2018 04:56 AM, Andy Shevchenko wrote:
>>> On Thu, Aug 2, 2018 at 10:57 PM, Tony Battersby <tonyb-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.org> wrote:
>>>> Remove code duplication in error messages.  It is now safe to pas a NULL
>>>> dev to dev_err(), so the checks to avoid doing so are no longer
>>>> necessary.
>>>>
>>>> Example:
>>>>
>>>> Error message with dev != NULL:
>>>>   mpt3sas 0000:02:00.0: dma_pool_destroy chain pool, (____ptrval____) busy
>>>>
>>>> Same error message with dev == NULL before patch:
>>>>   dma_pool_destroy chain pool, (____ptrval____) busy
>>>>
>>>> Same error message with dev == NULL after patch:
>>>>   (NULL device *): dma_pool_destroy chain pool, (____ptrval____) busy
>>> Have you checked a history of this?
>>>
>>> I'm pretty sure this was created in an order to avoid bad looking (and
>>> in some cases frightening) "NULL device *" part.
>>>
>>> If it it's the case, I would rather leave it as is, and even not the
>>> case, I'm slightly more bent to the current state.
>>>
>> I did.  "drivers/base/dmapool.c", later moved to "mm/dmapool.c", was
>> added in linux-2.6.3, for which dev_err() did not work will a NULL dev,
>> so the check was necessary back then.  I agree that the (NULL device *):
>> bit is ugly, but these messages should be printed only after a kernel
>> bug, so it is not like they will be making a regular appearance in
>> dmesg.  Considering that, I think that it is better to keep it simple.
>>
>
> My original unsubmitted patch used the following:
>
> +#define pool_err(pool, fmt, args...) \
> +       do { \
> +               if ((pool)->dev) \
> +                       dev_err((pool)->dev, fmt, args); \
> +               else \
> +                       pr_err(fmt, args); \
> +       } while (0)
>
> But then I decided to simplify it to just use dev_err().  I still have
> the old version.  When I submit v3 of the patchset, which would you prefer?

JFYI: git log --no-merges --grep 'NULL device \*'

P.S. I already shared my opinion on this anyway.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Andy Shevchenko]

On Fri, Aug 3, 2018 at 6:17 PM, Tony Battersby <tonyb@cybernetics.com> wrote:
> On 08/03/2018 09:41 AM, Tony Battersby wrote:
>> On 08/03/2018 04:56 AM, Andy Shevchenko wrote:
>>> On Thu, Aug 2, 2018 at 10:57 PM, Tony Battersby <tonyb@cybernetics.com> wrote:
>>>> Remove code duplication in error messages.  It is now safe to pas a NULL
>>>> dev to dev_err(), so the checks to avoid doing so are no longer
>>>> necessary.
>>>>
>>>> Example:
>>>>
>>>> Error message with dev != NULL:
>>>>   mpt3sas 0000:02:00.0: dma_pool_destroy chain pool, (____ptrval____) busy
>>>>
>>>> Same error message with dev == NULL before patch:
>>>>   dma_pool_destroy chain pool, (____ptrval____) busy
>>>>
>>>> Same error message with dev == NULL after patch:
>>>>   (NULL device *): dma_pool_destroy chain pool, (____ptrval____) busy
>>> Have you checked a history of this?
>>>
>>> I'm pretty sure this was created in an order to avoid bad looking (and
>>> in some cases frightening) "NULL device *" part.
>>>
>>> If it it's the case, I would rather leave it as is, and even not the
>>> case, I'm slightly more bent to the current state.
>>>
>> I did.  "drivers/base/dmapool.c", later moved to "mm/dmapool.c", was
>> added in linux-2.6.3, for which dev_err() did not work will a NULL dev,
>> so the check was necessary back then.  I agree that the (NULL device *):
>> bit is ugly, but these messages should be printed only after a kernel
>> bug, so it is not like they will be making a regular appearance in
>> dmesg.  Considering that, I think that it is better to keep it simple.
>>
>
> My original unsubmitted patch used the following:
>
> +#define pool_err(pool, fmt, args...) \
> +       do { \
> +               if ((pool)->dev) \
> +                       dev_err((pool)->dev, fmt, args); \
> +               else \
> +                       pr_err(fmt, args); \
> +       } while (0)
>
> But then I decided to simplify it to just use dev_err().  I still have
> the old version.  When I submit v3 of the patchset, which would you prefer?

JFYI: git log --no-merges --grep 'NULL device \*'

P.S. I already shared my opinion on this anyway.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Andy Shevchenko]

On Fri, Aug 3, 2018 at 6:59 PM, Andy Shevchenko
<andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Aug 3, 2018 at 6:17 PM, Tony Battersby <tonyb-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.org> wrote:

>> But then I decided to simplify it to just use dev_err().  I still have
>> the old version.  When I submit v3 of the patchset, which would you prefer?
>
> JFYI: git log --no-merges --grep 'NULL device \*'

Example:

commit b4ba97e76763c4e582e3af1079e220e93b1b0d76
Author: Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org>
Date:   Fri Aug 19 08:37:50 2016 +0100

   drm: Avoid calling dev_printk(.dev = NULL)



> P.S. I already shared my opinion on this anyway.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Andy Shevchenko]

On Fri, Aug 3, 2018 at 6:59 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Aug 3, 2018 at 6:17 PM, Tony Battersby <tonyb@cybernetics.com> wrote:

>> But then I decided to simplify it to just use dev_err().  I still have
>> the old version.  When I submit v3 of the patchset, which would you prefer?
>
> JFYI: git log --no-merges --grep 'NULL device \*'

Example:

commit b4ba97e76763c4e582e3af1079e220e93b1b0d76
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Fri Aug 19 08:37:50 2016 +0100

   drm: Avoid calling dev_printk(.dev = NULL)



> P.S. I already shared my opinion on this anyway.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Tony Battersby]

On 08/03/2018 12:01 PM, Andy Shevchenko wrote:
> On Fri, Aug 3, 2018 at 6:59 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Fri, Aug 3, 2018 at 6:17 PM, Tony Battersby <tonyb@cybernetics.com> wrote:
>>> But then I decided to simplify it to just use dev_err().  I still have
>>> the old version.  When I submit v3 of the patchset, which would you prefer?
>> JFYI: git log --no-merges --grep 'NULL device \*'
> Example:
>
> commit b4ba97e76763c4e582e3af1079e220e93b1b0d76
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Fri Aug 19 08:37:50 2016 +0100
>
>    drm: Avoid calling dev_printk(.dev = NULL)
>

Point taken.  I'll go with pool_err() on the next round.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Tony Battersby]

On 08/03/2018 12:01 PM, Andy Shevchenko wrote:
> On Fri, Aug 3, 2018 at 6:59 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Fri, Aug 3, 2018 at 6:17 PM, Tony Battersby <tonyb@cybernetics.com> wrote:
>>> But then I decided to simplify it to just use dev_err().  I still have
>>> the old version.  When I submit v3 of the patchset, which would you prefer?
>> JFYI: git log --no-merges --grep 'NULL device \*'
> Example:
>
> commit b4ba97e76763c4e582e3af1079e220e93b1b0d76
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Fri Aug 19 08:37:50 2016 +0100
>
>    drm: Avoid calling dev_printk(.dev = NULL)
>

Point taken.A  I'll go with pool_err() on the next round.

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Matthew Wilcox]

On Fri, Aug 03, 2018 at 06:59:20PM +0300, Andy Shevchenko wrote:
> >>> I'm pretty sure this was created in an order to avoid bad looking (and
> >>> in some cases frightening) "NULL device *" part.
> 
> JFYI: git log --no-merges --grep 'NULL device \*'

I think those commits actually argue in favour of Tony's patch to remove
the special casing.  Is it really useful to create dma pools with a NULL
device?

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Matthew Wilcox]

On Fri, Aug 03, 2018 at 06:59:20PM +0300, Andy Shevchenko wrote:
> >>> I'm pretty sure this was created in an order to avoid bad looking (and
> >>> in some cases frightening) "NULL device *" part.
> 
> JFYI: git log --no-merges --grep 'NULL device \*'

I think those commits actually argue in favour of Tony's patch to remove
the special casing.  Is it really useful to create dma pools with a NULL
device?

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Tony Battersby]

On 08/03/2018 12:22 PM, Matthew Wilcox wrote:
> On Fri, Aug 03, 2018 at 06:59:20PM +0300, Andy Shevchenko wrote:
>>>>> I'm pretty sure this was created in an order to avoid bad looking (and
>>>>> in some cases frightening) "NULL device *" part.
>> JFYI: git log --no-merges --grep 'NULL device \*'
> I think those commits actually argue in favour of Tony's patch to remove
> the special casing.  Is it really useful to create dma pools with a NULL
> device?
>
>
dma_alloc_coherent() does appear to support a NULL dev, so it might make
sense in theory.  But I can't find any in-tree callers that actually
pass a NULL dev to dma_pool_create().  So for one of the dreaded (NULL
device *) messages to show up, it would take both a new caller that
passes a NULL dev to dma_pool_create() and a bug to cause the message to
be printed.  Is that worth the special casing?

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Tony Battersby]

On 08/03/2018 12:22 PM, Matthew Wilcox wrote:
> On Fri, Aug 03, 2018 at 06:59:20PM +0300, Andy Shevchenko wrote:
>>>>> I'm pretty sure this was created in an order to avoid bad looking (and
>>>>> in some cases frightening) "NULL device *" part.
>> JFYI: git log --no-merges --grep 'NULL device \*'
> I think those commits actually argue in favour of Tony's patch to remove
> the special casing.  Is it really useful to create dma pools with a NULL
> device?
>
>
dma_alloc_coherent() does appear to support a NULL dev, so it might make
sense in theory.A  But I can't find any in-tree callers that actually
pass a NULL dev to dma_pool_create().A  So for one of the dreaded (NULL
device *) messages to show up, it would take both a new caller that
passes a NULL dev to dma_pool_create() and a bug to cause the message to
be printed.A  Is that worth the special casing?

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Andy Shevchenko]

On Fri, Aug 3, 2018 at 8:03 PM, Tony Battersby <tonyb-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.org> wrote:
> On 08/03/2018 12:22 PM, Matthew Wilcox wrote:
>> On Fri, Aug 03, 2018 at 06:59:20PM +0300, Andy Shevchenko wrote:
>>>>>> I'm pretty sure this was created in an order to avoid bad looking (and
>>>>>> in some cases frightening) "NULL device *" part.
>>> JFYI: git log --no-merges --grep 'NULL device \*'
>> I think those commits actually argue in favour of Tony's patch to remove
>> the special casing.  Is it really useful to create dma pools with a NULL
>> device?

> dma_alloc_coherent() does appear to support a NULL dev, so it might make
> sense in theory.  But I can't find any in-tree callers that actually
> pass a NULL dev to dma_pool_create().  So for one of the dreaded (NULL
> device *) messages to show up, it would take both a new caller that
> passes a NULL dev to dma_pool_create() and a bug to cause the message to
> be printed.  Is that worth the special casing?

So, then you need to rephrase the commit message explaining this
("NULL device is wrong to pass in the first place... bla bla bla").

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Andy Shevchenko]

On Fri, Aug 3, 2018 at 8:03 PM, Tony Battersby <tonyb@cybernetics.com> wrote:
> On 08/03/2018 12:22 PM, Matthew Wilcox wrote:
>> On Fri, Aug 03, 2018 at 06:59:20PM +0300, Andy Shevchenko wrote:
>>>>>> I'm pretty sure this was created in an order to avoid bad looking (and
>>>>>> in some cases frightening) "NULL device *" part.
>>> JFYI: git log --no-merges --grep 'NULL device \*'
>> I think those commits actually argue in favour of Tony's patch to remove
>> the special casing.  Is it really useful to create dma pools with a NULL
>> device?

> dma_alloc_coherent() does appear to support a NULL dev, so it might make
> sense in theory.  But I can't find any in-tree callers that actually
> pass a NULL dev to dma_pool_create().  So for one of the dreaded (NULL
> device *) messages to show up, it would take both a new caller that
> passes a NULL dev to dma_pool_create() and a bug to cause the message to
> be printed.  Is that worth the special casing?

So, then you need to rephrase the commit message explaining this
("NULL device is wrong to pass in the first place... bla bla bla").

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Tony Battersby]

On 08/03/2018 02:38 PM, Andy Shevchenko wrote:
>
>> dma_alloc_coherent() does appear to support a NULL dev, so it might make
>> sense in theory.  But I can't find any in-tree callers that actually
>> pass a NULL dev to dma_pool_create().  So for one of the dreaded (NULL
>> device *) messages to show up, it would take both a new caller that
>> passes a NULL dev to dma_pool_create() and a bug to cause the message to
>> be printed.  Is that worth the special casing?
> So, then you need to rephrase the commit message explaining this
> ("NULL device is wrong to pass in the first place... bla bla bla").
>
Will do.

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Tony Battersby]

On 08/03/2018 02:38 PM, Andy Shevchenko wrote:
>
>> dma_alloc_coherent() does appear to support a NULL dev, so it might make
>> sense in theory.  But I can't find any in-tree callers that actually
>> pass a NULL dev to dma_pool_create().  So for one of the dreaded (NULL
>> device *) messages to show up, it would take both a new caller that
>> passes a NULL dev to dma_pool_create() and a bug to cause the message to
>> be printed.  Is that worth the special casing?
> So, then you need to rephrase the commit message explaining this
> ("NULL device is wrong to pass in the first place... bla bla bla").
>
Will do.

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Douglas Gilbert]

On 2018-08-03 02:38 PM, Andy Shevchenko wrote:
> On Fri, Aug 3, 2018 at 8:03 PM, Tony Battersby <tonyb-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.org> wrote:
>> On 08/03/2018 12:22 PM, Matthew Wilcox wrote:
>>> On Fri, Aug 03, 2018 at 06:59:20PM +0300, Andy Shevchenko wrote:
>>>>>>> I'm pretty sure this was created in an order to avoid bad looking (and
>>>>>>> in some cases frightening) "NULL device *" part.
>>>> JFYI: git log --no-merges --grep 'NULL device \*'
>>> I think those commits actually argue in favour of Tony's patch to remove
>>> the special casing.  Is it really useful to create dma pools with a NULL
>>> device?
> 
>> dma_alloc_coherent() does appear to support a NULL dev, so it might make
>> sense in theory.  But I can't find any in-tree callers that actually
>> pass a NULL dev to dma_pool_create().  So for one of the dreaded (NULL
>> device *) messages to show up, it would take both a new caller that
>> passes a NULL dev to dma_pool_create() and a bug to cause the message to
>> be printed.  Is that worth the special casing?
> 
> So, then you need to rephrase the commit message explaining this
> ("NULL device is wrong to pass in the first place... bla bla bla").
> 

"Pre-condition(s)", you might use that term for non-obvious requirements
for a function. The assumption then is if it/they are violated that
your function won't work. It also implies your function does not check them.
One implicit pre-condition on almost all C functions that take a pointer:
that the pointer points to accessible memory.

Doug Gilbert

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Douglas Gilbert]

On 2018-08-03 02:38 PM, Andy Shevchenko wrote:
> On Fri, Aug 3, 2018 at 8:03 PM, Tony Battersby <tonyb@cybernetics.com> wrote:
>> On 08/03/2018 12:22 PM, Matthew Wilcox wrote:
>>> On Fri, Aug 03, 2018 at 06:59:20PM +0300, Andy Shevchenko wrote:
>>>>>>> I'm pretty sure this was created in an order to avoid bad looking (and
>>>>>>> in some cases frightening) "NULL device *" part.
>>>> JFYI: git log --no-merges --grep 'NULL device \*'
>>> I think those commits actually argue in favour of Tony's patch to remove
>>> the special casing.  Is it really useful to create dma pools with a NULL
>>> device?
> 
>> dma_alloc_coherent() does appear to support a NULL dev, so it might make
>> sense in theory.  But I can't find any in-tree callers that actually
>> pass a NULL dev to dma_pool_create().  So for one of the dreaded (NULL
>> device *) messages to show up, it would take both a new caller that
>> passes a NULL dev to dma_pool_create() and a bug to cause the message to
>> be printed.  Is that worth the special casing?
> 
> So, then you need to rephrase the commit message explaining this
> ("NULL device is wrong to pass in the first place... bla bla bla").
> 

"Pre-condition(s)", you might use that term for non-obvious requirements
for a function. The assumption then is if it/they are violated that
your function won't work. It also implies your function does not check them.
One implicit pre-condition on almost all C functions that take a pointer:
that the pointer points to accessible memory.

Doug Gilbert

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Tony Battersby]

On 08/03/2018 01:03 PM, Tony Battersby wrote:
> On 08/03/2018 12:22 PM, Matthew Wilcox wrote:
>> On Fri, Aug 03, 2018 at 06:59:20PM +0300, Andy Shevchenko wrote:
>>>>>> I'm pretty sure this was created in an order to avoid bad looking (and
>>>>>> in some cases frightening) "NULL device *" part.
>>> JFYI: git log --no-merges --grep 'NULL device \*'
>> I think those commits actually argue in favour of Tony's patch to remove
>> the special casing.  Is it really useful to create dma pools with a NULL
>> device?
>>
>>
> dma_alloc_coherent() does appear to support a NULL dev, so it might make
> sense in theory.  But I can't find any in-tree callers that actually
> pass a NULL dev to dma_pool_create().  So for one of the dreaded (NULL
> device *) messages to show up, it would take both a new caller that
> passes a NULL dev to dma_pool_create() and a bug to cause the message to
> be printed.  Is that worth the special casing?
>

Out of curiosity, I just tried to create a dmapool with a NULL dev and
it crashed on this:

static inline int dev_to_node(struct device *dev)
{
	return dev->numa_node;
}

struct dma_pool *dma_pool_create(const char *name, struct device *dev,
				 size_t size, size_t align, size_t boundary)
{
	...
	retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, dev_to_node(dev));
	...
}

So either it needs more special cases for supporting a NULL dev, or the
special cases can be removed since no one does that anyway.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Tony Battersby]

On 08/03/2018 01:03 PM, Tony Battersby wrote:
> On 08/03/2018 12:22 PM, Matthew Wilcox wrote:
>> On Fri, Aug 03, 2018 at 06:59:20PM +0300, Andy Shevchenko wrote:
>>>>>> I'm pretty sure this was created in an order to avoid bad looking (and
>>>>>> in some cases frightening) "NULL device *" part.
>>> JFYI: git log --no-merges --grep 'NULL device \*'
>> I think those commits actually argue in favour of Tony's patch to remove
>> the special casing.  Is it really useful to create dma pools with a NULL
>> device?
>>
>>
> dma_alloc_coherent() does appear to support a NULL dev, so it might make
> sense in theory.A  But I can't find any in-tree callers that actually
> pass a NULL dev to dma_pool_create().A  So for one of the dreaded (NULL
> device *) messages to show up, it would take both a new caller that
> passes a NULL dev to dma_pool_create() and a bug to cause the message to
> be printed.A  Is that worth the special casing?
>

Out of curiosity, I just tried to create a dmapool with a NULL dev and
it crashed on this:

static inline int dev_to_node(struct device *dev)
{
	return dev->numa_node;
}

struct dma_pool *dma_pool_create(const char *name, struct device *dev,
				 size_t size, size_t align, size_t boundary)
{
	...
	retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, dev_to_node(dev));
	...
}

So either it needs more special cases for supporting a NULL dev, or the
special cases can be removed since no one does that anyway.

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Matthew Wilcox]

On Fri, Aug 03, 2018 at 02:43:07PM -0400, Tony Battersby wrote:
> Out of curiosity, I just tried to create a dmapool with a NULL dev and
> it crashed on this:
> 
> static inline int dev_to_node(struct device *dev)
> {
> 	return dev->numa_node;
> }
> 
> struct dma_pool *dma_pool_create(const char *name, struct device *dev,
> 				 size_t size, size_t align, size_t boundary)
> {
> 	...
> 	retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, dev_to_node(dev));
> 	...
> }
> 
> So either it needs more special cases for supporting a NULL dev, or the
> special cases can be removed since no one does that anyway.

Actually, it's worse.  dev_to_node() works with a NULL dev ... unless
CONFIG_NUMA is set.  So we're leaving a timebomb by pretending to
allow it.  Let's just 'if (!dev) return NULL;' early in create.

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Matthew Wilcox]

On Fri, Aug 03, 2018 at 02:43:07PM -0400, Tony Battersby wrote:
> Out of curiosity, I just tried to create a dmapool with a NULL dev and
> it crashed on this:
> 
> static inline int dev_to_node(struct device *dev)
> {
> 	return dev->numa_node;
> }
> 
> struct dma_pool *dma_pool_create(const char *name, struct device *dev,
> 				 size_t size, size_t align, size_t boundary)
> {
> 	...
> 	retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, dev_to_node(dev));
> 	...
> }
> 
> So either it needs more special cases for supporting a NULL dev, or the
> special cases can be removed since no one does that anyway.

Actually, it's worse.  dev_to_node() works with a NULL dev ... unless
CONFIG_NUMA is set.  So we're leaving a timebomb by pretending to
allow it.  Let's just 'if (!dev) return NULL;' early in create.

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Tony Battersby]

On 08/03/2018 05:07 PM, Matthew Wilcox wrote:
> On Fri, Aug 03, 2018 at 02:43:07PM -0400, Tony Battersby wrote:
>> Out of curiosity, I just tried to create a dmapool with a NULL dev and
>> it crashed on this:
>>
>> static inline int dev_to_node(struct device *dev)
>> {
>> 	return dev->numa_node;
>> }
>>
>> struct dma_pool *dma_pool_create(const char *name, struct device *dev,
>> 				 size_t size, size_t align, size_t boundary)
>> {
>> 	...
>> 	retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, dev_to_node(dev));
>> 	...
>> }
>>
>> So either it needs more special cases for supporting a NULL dev, or the
>> special cases can be removed since no one does that anyway.
> Actually, it's worse.  dev_to_node() works with a NULL dev ... unless
> CONFIG_NUMA is set.  So we're leaving a timebomb by pretending to
> allow it.  Let's just 'if (!dev) return NULL;' early in create.
>
>
Looking further down it does stuff with dev->dma_pools unconditionally
that doesn't depend on the config.  So it would blow up on non-NUMA
also.  So no timebomb, just an immediate kaboom.

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/9] dmapool: cleanup error messages

[Tony Battersby]

On 08/03/2018 05:07 PM, Matthew Wilcox wrote:
> On Fri, Aug 03, 2018 at 02:43:07PM -0400, Tony Battersby wrote:
>> Out of curiosity, I just tried to create a dmapool with a NULL dev and
>> it crashed on this:
>>
>> static inline int dev_to_node(struct device *dev)
>> {
>> 	return dev->numa_node;
>> }
>>
>> struct dma_pool *dma_pool_create(const char *name, struct device *dev,
>> 				 size_t size, size_t align, size_t boundary)
>> {
>> 	...
>> 	retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, dev_to_node(dev));
>> 	...
>> }
>>
>> So either it needs more special cases for supporting a NULL dev, or the
>> special cases can be removed since no one does that anyway.
> Actually, it's worse.  dev_to_node() works with a NULL dev ... unless
> CONFIG_NUMA is set.  So we're leaving a timebomb by pretending to
> allow it.  Let's just 'if (!dev) return NULL;' early in create.
>
>
Looking further down it does stuff with dev->dma_pools unconditionally
that doesn't depend on the config.A  So it would blow up on non-NUMA
also.A  So no timebomb, just an immediate kaboom.