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

[Tony Battersby <tonyb@cybernetics.com>]

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?