Re: [PATCH v2 2/9] dmapool: cleanup error messages
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
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.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/9] dmapool: cleanup error messages
On 2018-08-03 02:38 PM, Andy Shevchenko wrote:
On Fri, Aug 3, 2018 at 8: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?
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/9] dmapool: cleanup error messages
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.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/9] dmapool: cleanup error messages
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
On Fri, Aug 3, 2018 at 8: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?
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/9] dmapool: cleanup error messages
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
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? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/9] dmapool: cleanup error messages
On 08/03/2018 12:01 PM, Andy Shevchenko wrote: > On Fri, Aug 3, 2018 at 6:59 PM, Andy Shevchenko > wrote: >> On Fri, Aug 3, 2018 at 6:17 PM, Tony Battersby 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 > 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
On Fri, Aug 3, 2018 at 6:59 PM, Andy Shevchenko wrote: > On Fri, Aug 3, 2018 at 6:17 PM, Tony Battersby 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 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/9] dmapool: cleanup error messages
On Fri, Aug 3, 2018 at 6:17 PM, Tony Battersby 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
>>> 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 :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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/9] dmapool: cleanup error messages
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
>> 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 :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
On 08/03/2018 04:56 AM, Andy Shevchenko wrote: > On Thu, Aug 2, 2018 at 10:57 PM, Tony Battersby 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 :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
On Thu, Aug 2, 2018 at 10:57 PM, Tony Battersby 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 :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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 2/9] dmapool: cleanup error messages
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 :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
---
--- linux/mm/dmapool.c.orig 2018-08-02 09:54:25.0 -0400
+++ linux/mm/dmapool.c 2018-08-02 09:57:58.0 -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;
}
}
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
