* [PATCH v2 2/9] dmapool: cleanup error messages
@ 2018-08-02 19:57 ` Tony Battersby
0 siblings, 0 replies; 30+ messages in thread
From: Tony Battersby @ 2018-08-02 19:57 UTC (permalink / raw)
To: Matthew Wilcox, Christoph Hellwig, Marek Szyprowski,
Sathya Prakash, Chaitra P B, Suganath Prabu Subramani,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-mm,
linux-scsi-u79uwXL29TY76Z2rM5mHXA,
MPT-FusionLinux.pdl-dY08KVG/lbpWk0Htik3J/w
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;
}
}
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 2/9] dmapool: cleanup error messages
@ 2018-08-02 19:57 ` Tony Battersby
0 siblings, 0 replies; 30+ messages in thread
From: Tony Battersby @ 2018-08-02 19:57 UTC (permalink / raw)
To: Matthew Wilcox, Christoph Hellwig, Marek Szyprowski,
Sathya Prakash, Chaitra P B, Suganath Prabu Subramani, iommu,
linux-mm, linux-scsi, MPT-FusionLinux.pdl
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;
}
}
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
2018-08-02 19:57 ` Tony Battersby
@ 2018-08-03 8:56 ` Andy Shevchenko
-1 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2018-08-03 8:56 UTC (permalink / raw)
To: Tony Battersby
Cc: linux-scsi, Chaitra P B, Suganath Prabu Subramani,
Sathya Prakash, Matthew Wilcox, linux-mm,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
MPT-FusionLinux.pdl-dY08KVG/lbpWk0Htik3J/w, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
@ 2018-08-03 8:56 ` Andy Shevchenko
0 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2018-08-03 8:56 UTC (permalink / raw)
To: Tony Battersby
Cc: Matthew Wilcox, Christoph Hellwig, Marek Szyprowski,
Sathya Prakash, Chaitra P B, Suganath Prabu Subramani, iommu,
linux-mm, linux-scsi, MPT-FusionLinux.pdl
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
2018-08-03 8:56 ` Andy Shevchenko
@ 2018-08-03 13:41 ` Tony Battersby
-1 siblings, 0 replies; 30+ messages in thread
From: Tony Battersby @ 2018-08-03 13:41 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-scsi, Chaitra P B, Suganath Prabu Subramani,
Sathya Prakash, Matthew Wilcox, linux-mm,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
MPT-FusionLinux.pdl-dY08KVG/lbpWk0Htik3J/w, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
@ 2018-08-03 13:41 ` Tony Battersby
0 siblings, 0 replies; 30+ messages in thread
From: Tony Battersby @ 2018-08-03 13:41 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Matthew Wilcox, Christoph Hellwig, Marek Szyprowski,
Sathya Prakash, Chaitra P B, Suganath Prabu Subramani, iommu,
linux-mm, linux-scsi, MPT-FusionLinux.pdl
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
2018-08-03 13:41 ` Tony Battersby
@ 2018-08-03 15:17 ` Tony Battersby
-1 siblings, 0 replies; 30+ messages in thread
From: Tony Battersby @ 2018-08-03 15:17 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-scsi, Chaitra P B, Suganath Prabu Subramani,
Sathya Prakash, Matthew Wilcox, linux-mm,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
MPT-FusionLinux.pdl-dY08KVG/lbpWk0Htik3J/w, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
@ 2018-08-03 15:17 ` Tony Battersby
0 siblings, 0 replies; 30+ messages in thread
From: Tony Battersby @ 2018-08-03 15:17 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Matthew Wilcox, Christoph Hellwig, Marek Szyprowski,
Sathya Prakash, Chaitra P B, Suganath Prabu Subramani, iommu,
linux-mm, linux-scsi, MPT-FusionLinux.pdl
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?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
2018-08-03 15:17 ` Tony Battersby
@ 2018-08-03 15:59 ` Andy Shevchenko
-1 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2018-08-03 15:59 UTC (permalink / raw)
To: Tony Battersby
Cc: linux-scsi, Chaitra P B, Suganath Prabu Subramani,
Sathya Prakash, Matthew Wilcox, linux-mm,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
MPT-FusionLinux.pdl-dY08KVG/lbpWk0Htik3J/w, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
@ 2018-08-03 15:59 ` Andy Shevchenko
0 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2018-08-03 15:59 UTC (permalink / raw)
To: Tony Battersby
Cc: Matthew Wilcox, Christoph Hellwig, Marek Szyprowski,
Sathya Prakash, Chaitra P B, Suganath Prabu Subramani, iommu,
linux-mm, linux-scsi, MPT-FusionLinux.pdl
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
2018-08-03 15:59 ` Andy Shevchenko
@ 2018-08-03 16:01 ` Andy Shevchenko
-1 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2018-08-03 16:01 UTC (permalink / raw)
To: Tony Battersby
Cc: linux-scsi, Chaitra P B, Suganath Prabu Subramani,
Sathya Prakash, Matthew Wilcox, linux-mm,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
MPT-FusionLinux.pdl-dY08KVG/lbpWk0Htik3J/w, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
@ 2018-08-03 16:01 ` Andy Shevchenko
0 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2018-08-03 16:01 UTC (permalink / raw)
To: Tony Battersby
Cc: Matthew Wilcox, Christoph Hellwig, Marek Szyprowski,
Sathya Prakash, Chaitra P B, Suganath Prabu Subramani, iommu,
linux-mm, linux-scsi, MPT-FusionLinux.pdl
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
2018-08-03 16:01 ` Andy Shevchenko
@ 2018-08-03 16:10 ` Tony Battersby
-1 siblings, 0 replies; 30+ messages in thread
From: Tony Battersby @ 2018-08-03 16:10 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-scsi, Chaitra P B, Suganath Prabu Subramani,
Sathya Prakash, Matthew Wilcox, linux-mm,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
MPT-FusionLinux.pdl-dY08KVG/lbpWk0Htik3J/w, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
@ 2018-08-03 16:10 ` Tony Battersby
0 siblings, 0 replies; 30+ messages in thread
From: Tony Battersby @ 2018-08-03 16:10 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Matthew Wilcox, Christoph Hellwig, Marek Szyprowski,
Sathya Prakash, Chaitra P B, Suganath Prabu Subramani, iommu,
linux-mm, linux-scsi, MPT-FusionLinux.pdl
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
2018-08-03 15:59 ` Andy Shevchenko
@ 2018-08-03 16:22 ` Matthew Wilcox
-1 siblings, 0 replies; 30+ messages in thread
From: Matthew Wilcox @ 2018-08-03 16:22 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-scsi, Chaitra P B, Suganath Prabu Subramani,
Sathya Prakash, Christoph Hellwig, linux-mm,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
MPT-FusionLinux.pdl-dY08KVG/lbpWk0Htik3J/w, Tony Battersby
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?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
@ 2018-08-03 16:22 ` Matthew Wilcox
0 siblings, 0 replies; 30+ messages in thread
From: Matthew Wilcox @ 2018-08-03 16:22 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Tony Battersby, Christoph Hellwig, Marek Szyprowski,
Sathya Prakash, Chaitra P B, Suganath Prabu Subramani, iommu,
linux-mm, linux-scsi, MPT-FusionLinux.pdl
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?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
2018-08-03 16:22 ` Matthew Wilcox
@ 2018-08-03 17:03 ` Tony Battersby
-1 siblings, 0 replies; 30+ messages in thread
From: Tony Battersby @ 2018-08-03 17:03 UTC (permalink / raw)
To: Matthew Wilcox, Andy Shevchenko
Cc: linux-scsi, Chaitra P B, Suganath Prabu Subramani,
Sathya Prakash, linux-mm,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
MPT-FusionLinux.pdl-dY08KVG/lbpWk0Htik3J/w, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
@ 2018-08-03 17:03 ` Tony Battersby
0 siblings, 0 replies; 30+ messages in thread
From: Tony Battersby @ 2018-08-03 17:03 UTC (permalink / raw)
To: Matthew Wilcox, Andy Shevchenko
Cc: Christoph Hellwig, Marek Szyprowski, Sathya Prakash, Chaitra P B,
Suganath Prabu Subramani, iommu, linux-mm, linux-scsi,
MPT-FusionLinux.pdl
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?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
2018-08-03 17:03 ` Tony Battersby
@ 2018-08-03 18:38 ` Andy Shevchenko
-1 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2018-08-03 18:38 UTC (permalink / raw)
To: Tony Battersby
Cc: linux-scsi, Chaitra P B, Suganath Prabu Subramani,
Sathya Prakash, Matthew Wilcox, linux-mm,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
MPT-FusionLinux.pdl-dY08KVG/lbpWk0Htik3J/w, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
@ 2018-08-03 18:38 ` Andy Shevchenko
0 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2018-08-03 18:38 UTC (permalink / raw)
To: Tony Battersby
Cc: Matthew Wilcox, Christoph Hellwig, Marek Szyprowski,
Sathya Prakash, Chaitra P B, Suganath Prabu Subramani, iommu,
linux-mm, linux-scsi, MPT-FusionLinux.pdl
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
2018-08-03 17:03 ` Tony Battersby
@ 2018-08-03 18:43 ` Tony Battersby
-1 siblings, 0 replies; 30+ messages in thread
From: Tony Battersby @ 2018-08-03 18:43 UTC (permalink / raw)
To: Matthew Wilcox, Andy Shevchenko
Cc: linux-scsi, Chaitra P B, Suganath Prabu Subramani,
Sathya Prakash, linux-mm,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
MPT-FusionLinux.pdl-dY08KVG/lbpWk0Htik3J/w, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
@ 2018-08-03 18:43 ` Tony Battersby
0 siblings, 0 replies; 30+ messages in thread
From: Tony Battersby @ 2018-08-03 18:43 UTC (permalink / raw)
To: Matthew Wilcox, Andy Shevchenko
Cc: Christoph Hellwig, Marek Szyprowski, Sathya Prakash, Chaitra P B,
Suganath Prabu Subramani, iommu, linux-mm, linux-scsi,
MPT-FusionLinux.pdl
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
2018-08-03 18:38 ` Andy Shevchenko
@ 2018-08-03 18:44 ` Tony Battersby
-1 siblings, 0 replies; 30+ messages in thread
From: Tony Battersby @ 2018-08-03 18:44 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-scsi, Chaitra P B, Suganath Prabu Subramani,
Sathya Prakash, Matthew Wilcox, linux-mm,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
MPT-FusionLinux.pdl-dY08KVG/lbpWk0Htik3J/w, Christoph Hellwig
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
@ 2018-08-03 18:44 ` Tony Battersby
0 siblings, 0 replies; 30+ messages in thread
From: Tony Battersby @ 2018-08-03 18:44 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Matthew Wilcox, Christoph Hellwig, Marek Szyprowski,
Sathya Prakash, Chaitra P B, Suganath Prabu Subramani, iommu,
linux-mm, linux-scsi, MPT-FusionLinux.pdl
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
2018-08-03 18:38 ` Andy Shevchenko
@ 2018-08-03 19:07 ` Douglas Gilbert
-1 siblings, 0 replies; 30+ messages in thread
From: Douglas Gilbert @ 2018-08-03 19:07 UTC (permalink / raw)
To: Andy Shevchenko, Tony Battersby
Cc: linux-scsi, Chaitra P B, Suganath Prabu Subramani,
Sathya Prakash, Matthew Wilcox, linux-mm,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
MPT-FusionLinux.pdl-dY08KVG/lbpWk0Htik3J/w, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
@ 2018-08-03 19:07 ` Douglas Gilbert
0 siblings, 0 replies; 30+ messages in thread
From: Douglas Gilbert @ 2018-08-03 19:07 UTC (permalink / raw)
To: Andy Shevchenko, Tony Battersby
Cc: Matthew Wilcox, Christoph Hellwig, Marek Szyprowski,
Sathya Prakash, Chaitra P B, Suganath Prabu Subramani, iommu,
linux-mm, linux-scsi, MPT-FusionLinux.pdl
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
2018-08-03 18:43 ` Tony Battersby
@ 2018-08-03 21:07 ` Matthew Wilcox
-1 siblings, 0 replies; 30+ messages in thread
From: Matthew Wilcox @ 2018-08-03 21:07 UTC (permalink / raw)
To: Tony Battersby
Cc: linux-scsi, Chaitra P B, Suganath Prabu Subramani,
Sathya Prakash,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-mm,
Andy Shevchenko, MPT-FusionLinux.pdl-dY08KVG/lbpWk0Htik3J/w,
Christoph Hellwig
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
@ 2018-08-03 21:07 ` Matthew Wilcox
0 siblings, 0 replies; 30+ messages in thread
From: Matthew Wilcox @ 2018-08-03 21:07 UTC (permalink / raw)
To: Tony Battersby
Cc: Andy Shevchenko, Christoph Hellwig, Marek Szyprowski,
Sathya Prakash, Chaitra P B, Suganath Prabu Subramani, iommu,
linux-mm, linux-scsi, MPT-FusionLinux.pdl
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
2018-08-03 21:07 ` Matthew Wilcox
@ 2018-08-03 21:18 ` Tony Battersby
-1 siblings, 0 replies; 30+ messages in thread
From: Tony Battersby @ 2018-08-03 21:18 UTC (permalink / raw)
To: Matthew Wilcox
Cc: linux-scsi, Chaitra P B, Suganath Prabu Subramani,
Sathya Prakash,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-mm,
Andy Shevchenko, MPT-FusionLinux.pdl-dY08KVG/lbpWk0Htik3J/w,
Christoph Hellwig
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/9] dmapool: cleanup error messages
@ 2018-08-03 21:18 ` Tony Battersby
0 siblings, 0 replies; 30+ messages in thread
From: Tony Battersby @ 2018-08-03 21:18 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Andy Shevchenko, Christoph Hellwig, Marek Szyprowski,
Sathya Prakash, Chaitra P B, Suganath Prabu Subramani, iommu,
linux-mm, linux-scsi, MPT-FusionLinux.pdl
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2018-08-03 21:18 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-02 19:57 [PATCH v2 2/9] dmapool: cleanup error messages Tony Battersby
2018-08-02 19:57 ` Tony Battersby
[not found] ` <a9f7ca9a-38d5-12e2-7d15-ab026425e85a-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.org>
2018-08-03 8:56 ` Andy Shevchenko
2018-08-03 8:56 ` Andy Shevchenko
[not found] ` <CAHp75Ve0su_S3ZWTtUEUohrs-iPiD1uzFOHhesLrWzJPOa2LNg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-08-03 13:41 ` Tony Battersby
2018-08-03 13:41 ` Tony Battersby
[not found] ` <7a943124-c65e-f0ed-cc5c-20b23f021505-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.org>
2018-08-03 15:17 ` Tony Battersby
2018-08-03 15:17 ` Tony Battersby
[not found] ` <b8547f8d-ac88-3d7b-9c2d-60a2f779259e-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.org>
2018-08-03 15:59 ` Andy Shevchenko
2018-08-03 15:59 ` Andy Shevchenko
[not found] ` <CAHp75VcoLVkp+BkFBLSqn95=3SaV-zr8cO1eSoQsrzZtJZESNQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-08-03 16:01 ` Andy Shevchenko
2018-08-03 16:01 ` Andy Shevchenko
[not found] ` <CAHp75VdkFfND+Mr+L96kkGEF7K49Fr2HWezQQ3DBOQvxTLjBcw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-08-03 16:10 ` Tony Battersby
2018-08-03 16:10 ` Tony Battersby
2018-08-03 16:22 ` Matthew Wilcox
2018-08-03 16:22 ` Matthew Wilcox
[not found] ` <20180803162212.GA4718-PfSpb0PWhxZc2C7mugBRk2EX/6BAtgUQ@public.gmane.org>
2018-08-03 17:03 ` Tony Battersby
2018-08-03 17:03 ` Tony Battersby
[not found] ` <a2e9e4fd-2aab-bc7e-8dbb-db4ece8cd84f-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.org>
2018-08-03 18:38 ` Andy Shevchenko
2018-08-03 18:38 ` Andy Shevchenko
[not found] ` <CAHp75VfZfhHS1Hgrm+3xJL=3gT9Bri16JJSFUJpDY0=Ev5X-PA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-08-03 18:44 ` Tony Battersby
2018-08-03 18:44 ` Tony Battersby
2018-08-03 19:07 ` Douglas Gilbert
2018-08-03 19:07 ` Douglas Gilbert
2018-08-03 18:43 ` Tony Battersby
2018-08-03 18:43 ` Tony Battersby
[not found] ` <f0762902-8f28-82eb-b871-337c2da290cf-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.org>
2018-08-03 21:07 ` Matthew Wilcox
2018-08-03 21:07 ` Matthew Wilcox
[not found] ` <20180803210745.GB9329-PfSpb0PWhxZc2C7mugBRk2EX/6BAtgUQ@public.gmane.org>
2018-08-03 21:18 ` Tony Battersby
2018-08-03 21:18 ` Tony Battersby
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.