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

[Yijing Wang <wangyijing-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>]

>>>  #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",