Re: [Patch Part3 V5 1/8] iommu/vt-d: Introduce helper function dmar_walk_resources()

From: Yijing Wang
Date: Tue Sep 16 2014 - 04:08:51 EST


>>> #include "irq_remapping.h"
>>>
>>> +typedef int (*dmar_res_handler_t)(struct acpi_dmar_header *, void *);
>>> +struct dmar_res_callback {
>>> + dmar_res_handler_t cb[ACPI_DMAR_TYPE_RESERVED];
>>> + void *arg[ACPI_DMAR_TYPE_RESERVED];
>>> + bool ignore_unhandled;
>>> + bool print_entry;
>>
>> Why do we need a switch to control print ?
> We will walk DMAR entries several times during hotplug and only
> want to print once.

Fine, thanks for your explanation.

>
>>
>>> +};
>>> +
>>>
>>> +static int dmar_walk_resources(struct acpi_dmar_header *start, size_t len,
>>> + struct dmar_res_callback *cb)
>>
>> The name dmar_walk_resources easily make people think this is related with I/O or memory resources.
>> Do you have a better idea of this ? What about dmar_walk_remapping_entry() or dmar_walk_remapping_structure() ?
> Good suggestion, I like dmar_walk_remapping_entries().
>>
>>> +{
>>> + int ret = 0;
>>> + struct acpi_dmar_header *iter, *next;
>>> + struct acpi_dmar_header *end = ((void *)start) + len;
>>> +
>>> + for (iter = start; iter < end && ret == 0; iter = next) {
>>> + next = (void *)iter + iter->length;
>>> + if (iter->length == 0) {
>>> + /* Avoid looping forever on bad ACPI tables */
>>> + pr_debug(FW_BUG "Invalid 0-length structure\n");
>>
>> What about use pr_warn() instead of pr_debug(), pr_debug() default is off.
> It seems a common practice for BIOS engineer to mark the last entry with
> zero length. So it will be annoying if we generate this debug message
> on product kernel.

OK.

>
>>
>>> + break;
>>> + } else if (next > end) {
>>> + /* Avoid passing table end */
>>> + pr_warn(FW_BUG "record passes table end\n");
>>> + ret = -EINVAL;
>>> + break;
>>> + }
>>> +
>>> + if (cb->print_entry)
>>> + dmar_table_print_dmar_entry(iter);
>>> +
>>> + if (iter->type >= ACPI_DMAR_TYPE_RESERVED) {
>>> + /* continue for forward compatibility */
>>> + pr_debug("Unknown DMAR structure type %d\n",
>>> + iter->type);
>>
>> Same as above.
> This is typically caused by new DMAR specification. It will also be
> annoying too if we also generate this debug message on an newer
> hardware platform with older linux kernel.

OK.

>
>>
>>> + } else if (cb->cb[iter->type]) {
>>> + ret = cb->cb[iter->type](iter, cb->arg[iter->type]);
>>> + } else if (!cb->ignore_unhandled) {
>>> + pr_warn("No handler for DMAR structure type %d\n",

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/