On 2018-01-22 22:35, Gustavo Lima Chaves wrote:
> * Jan Kiszka <[email protected]> [2018-01-20 12:06:53 +0100]:
>
> [...]
>
>>> + cap = pci_get_cap(device, PCI_CAP_PCIE | JAILHOUSE_PCI_EXT_CAP);
>>> + if (!cap)
>>> + return;
>>> +
>>> + val = pci_read_config(device->info->bdf,
>>
>> Let's cache bdf in local var, will give us shorter statements.
>
> True, sorry for that.
>
>>
>>> + cap->start + PCIE_DEV_CAPS_REG, 4);
>>> +
>>> + if (!(val & PCI_DEV_CAPS_FLR)) {
>>> + printk("Skipping function level reset for "
>>> + "device %02x:%02x.%x: no support\n",
>>> + PCI_BDF_PARAMS(device->info->bdf));
>>
>> Why warn here, but not if the device isn't PCIe-capable?
>
> Because these days it is kinda unlikely to fall in that case, I guess.
> Should I log on both, then?
Yes, that's better. If you pre-init val to 0, you can keep the test and
output above for both cases.
>
>>
>>> + return;
>>
>> If FLR is not supported, we should still do the silencing of the device
>> via the command register below. Just move the command reg update before
>> all the FLR-related logic.
>
> Have you tried that on devices without the PCI_DEV_CAPS_FLR bit set?
> At least here you get a Unsupported Request right away (and thus an
> AER). At least I understand from the spec that if reset is not
> supported, you should do nothing in that regard.
I'm also talking about devices without PCIe capability. All those
devices should at least get their DMA turned off (via the command
register) when we do a "reset" on them.
>
> [...]
>
>>> }
>>> }
>>> diff --git a/include/jailhouse/cell-config.h
>>> b/include/jailhouse/cell-config.h
>>> index cf50ef84..72c4facc 100644
>>> --- a/include/jailhouse/cell-config.h
>>> +++ b/include/jailhouse/cell-config.h
>>> @@ -141,6 +141,8 @@ struct jailhouse_pci_device {
>>> __u16 domain;
>>> __u16 bdf;
>>> __u32 bar_mask[6];
>>> + /** A cycle-count based value that Jailhouse will wait for the device
>>> to terminate any PCI transactions during function-level reset. This *won't*
>>> be filled up by jailhouse-config-create tool automatically, so tailor this
>>> to your devices and system. */
>>
>> Overlong line.
>
> I know, but is that a doxygen style comment? From my time playing with
> it, struct field documentation in Doxygen did not go well with new
> lines... or are there no Doxygen intentions there?
TBH, I didn't try recently, but we already have comment patterns
/** start of long comment
* that ends in a second line. */
So let's assume Doxygen can handle that by now.
>
>>
>> And we do need a reasonable default, set by config-create. So drop that
>> part of the comment.
>
> Ok, I'l think of something.
>
Thanks,
Jan
--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux
--
You received this message because you are subscribed to the Google Groups
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
