[PATCH] accel/qaic: Add crashdump to Sahara

[Jeffrey Hugo]

On 10/15/2024 1:04 PM, Bjorn Andersson wrote:
> On Tue, Oct 15, 2024 at 12:34:29PM -0600, Jeffrey Hugo wrote:
>> On 10/14/2024 3:52 PM, Bjorn Andersson wrote:
>>> On Wed, Sep 18, 2024 at 09:52:54AM -0600, Jeffrey Hugo wrote:
>>>> +	dev_table_entry = (struct sahara_debug_table_entry64 *)(context->rx);
>>>> +	for (i = 0; i < table_nents; ++i, ++image_out_table_entry, ++dev_table_entry) {
>>>> +		image_out_table_entry->type = le64_to_cpu(dev_table_entry->type);
>>>> +		image_out_table_entry->address = le64_to_cpu(dev_table_entry->address);
>>>> +		image_out_table_entry->length = le64_to_cpu(dev_table_entry->length);
>>>> +		strscpy(image_out_table_entry->description, dev_table_entry->description,
>>>> +			SAHARA_TABLE_ENTRY_STR_LEN);
>>>> +		strscpy(image_out_table_entry->filename,
>>>> +			dev_table_entry->filename,
>>>> +			SAHARA_TABLE_ENTRY_STR_LEN);
>>>> +	}
>>>> +
>>>> +	context->mem_dump_freespace = image_out_table_entry;
>>>> +
>>>> +	/* Done parsing the table, switch to image dump mode */
>>>> +	context->dump_table_length = 0;
>>>> +
>>>> +	/* Request the first chunk of the first image */
>>>> +	context->dump_image = (struct sahara_dump_table_entry *)(context->mem_dump +
>>>> +								sizeof(*dump_meta));
>>>
>>> I would have preferred to see this (and above) written such that it's
>>> explicitly clear that you're filling out an array of entries and then
>>> point this to the first entry in that array.
>>
>> I'm not sure I understand what you would like to see here.  Can you perhaps
>> give an example?
>>
> 
> Per your devcoredump definition at the top, image_out_table_entry is an
> array of struct sahara_dump_table_entry, which you fill out by sliding a
> pointer starting at mem_dump + sizeof(*dump_meta).
> 
> You then have context->dump_image to be a pointer to each element in
> this array, except that it's not expressed as an array...
> 
> But it took me a minute to understand that this was what the code is
> doing.
> 
> If you instead wrote it as:
> 
>    for (i = 0..table_nents) {
>    	image_out_table[i].foo = bar;
> 	...;
>    }
> 
>    context->dump_image = &image_out_table[0];
> 
> (Or perhaps even make dump_image an index into image_out_table)
> 
> It would have been obvious to me when I looked at the code that you're
> setting up an array and then looping over each entry in the array.
> 
> 
> So, I don't see anything wrong with the logic, but it would have been
> easier for me if the code manifested this array, as an array...
> 
> Perhaps I'm missing some detail which complicates things, as far as I
> can tell the logic presented is reasonable.

Ok, I see what you mean, and I believe its possible to transform the 
logic to use array notation in the loop as you suggest.

-Jeff