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@xxxxxxxxxxxxxxx> 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?
