Return -ENOSPC instead of -EOI when the status reports the NOSPC bit.
This signifies to the pci_endpoint test to skip this test instead of
reporting a failure.
Link: https://lore.kernel.org/linux-pci/20260317152707.GA85951@bhelgaas/T/#m87e4c24173097a0ea70195b71aab294ad8d6c283
Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
---
drivers/misc/pci_endpoint_test.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 55e128ed82f00ae13b6fe9768cdbe56adbe8f9da..34ba06fb53f04e48c1c05f4aae85e6ecd03ef447 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -61,6 +61,7 @@
#define STATUS_BAR_SUBRANGE_SETUP_FAIL BIT(15)
#define STATUS_BAR_SUBRANGE_CLEAR_SUCCESS BIT(16)
#define STATUS_BAR_SUBRANGE_CLEAR_FAIL BIT(17)
+#define STATUS_BAR_SUBRANGE_SETUP_NOSPC BIT(18)
#define PCI_ENDPOINT_TEST_LOWER_SRC_ADDR 0x0c
#define PCI_ENDPOINT_TEST_UPPER_SRC_ADDR 0x10
@@ -476,8 +477,11 @@ static int pci_endpoint_test_bar_subrange_cmd(struct pci_endpoint_test *test,
return -ETIMEDOUT;
status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
- if (status & fail_bit)
+ if (status & fail_bit) {
+ if (status & STATUS_BAR_SUBRANGE_SETUP_NOSPC)
+ return -ENOSPC;
return -EIO;
+ }
if (!(status & ok_bit))
return -EIO;
--
2.34.1On Wed, Mar 18, 2026 at 03:46:29PM +0100, Christian Bruel wrote:
> Return -ENOSPC instead of -EOI when the status reports the NOSPC bit.
s/EOI/EIO/
> This signifies to the pci_endpoint test to skip this test instead of
> reporting a failure.
>
> Link: https://lore.kernel.org/linux-pci/20260317152707.GA85951@bhelgaas/T/#m87e4c24173097a0ea70195b71aab294ad8d6c283
I would drop the link.
I would put this patch after pci_epf_test.c patch and before selftest patch.
> Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
> ---
> drivers/misc/pci_endpoint_test.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 55e128ed82f00ae13b6fe9768cdbe56adbe8f9da..34ba06fb53f04e48c1c05f4aae85e6ecd03ef447 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -61,6 +61,7 @@
> #define STATUS_BAR_SUBRANGE_SETUP_FAIL BIT(15)
> #define STATUS_BAR_SUBRANGE_CLEAR_SUCCESS BIT(16)
> #define STATUS_BAR_SUBRANGE_CLEAR_FAIL BIT(17)
> +#define STATUS_BAR_SUBRANGE_SETUP_NOSPC BIT(18)
>
> #define PCI_ENDPOINT_TEST_LOWER_SRC_ADDR 0x0c
> #define PCI_ENDPOINT_TEST_UPPER_SRC_ADDR 0x10
> @@ -476,8 +477,11 @@ static int pci_endpoint_test_bar_subrange_cmd(struct pci_endpoint_test *test,
> return -ETIMEDOUT;
>
> status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
> - if (status & fail_bit)
> + if (status & fail_bit) {
> + if (status & STATUS_BAR_SUBRANGE_SETUP_NOSPC)
> + return -ENOSPC;
Perhaps this should be something like:
if (command == COMMAND_BAR_SUBRANGE_SETUP && status & STATUS_BAR_SUBRANGE_SETUP_SKIP)
Personally, I'm not a big fan of having a common function for both
pci_endpoint_test_bar_subrange_setup() and
pci_endpoint_test_bar_subrange_clear() and then sending in parameters
for which bits to check. It make it hard to have small differences
between them (like checking for an bit that is only valid for one of
the commands).
I can understand why Koichiro wrote it why he did, but since we now
want differences, perhaps add a preparation patch that makes it two
separate functions?
It is not like pci_endpoint_test_bar_subrange_cmd() is a big function
anyway.
Kind regards,
Niklas
Hello,
>>
>> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
>> index 55e128ed82f00ae13b6fe9768cdbe56adbe8f9da..34ba06fb53f04e48c1c05f4aae85e6ecd03ef447 100644
>> --- a/drivers/misc/pci_endpoint_test.c
>> +++ b/drivers/misc/pci_endpoint_test.c
>> @@ -61,6 +61,7 @@
>> #define STATUS_BAR_SUBRANGE_SETUP_FAIL BIT(15)
>> #define STATUS_BAR_SUBRANGE_CLEAR_SUCCESS BIT(16)
>> #define STATUS_BAR_SUBRANGE_CLEAR_FAIL BIT(17)
>> +#define STATUS_BAR_SUBRANGE_SETUP_NOSPC BIT(18)
>>
>> #define PCI_ENDPOINT_TEST_LOWER_SRC_ADDR 0x0c
>> #define PCI_ENDPOINT_TEST_UPPER_SRC_ADDR 0x10
>> @@ -476,8 +477,11 @@ static int pci_endpoint_test_bar_subrange_cmd(struct pci_endpoint_test *test,
>> return -ETIMEDOUT;
>>
>> status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
>> - if (status & fail_bit)
>> + if (status & fail_bit) {
>> + if (status & STATUS_BAR_SUBRANGE_SETUP_NOSPC)
>> + return -ENOSPC;
>
> Perhaps this should be something like:
>
> if (command == COMMAND_BAR_SUBRANGE_SETUP && status & STATUS_BAR_SUBRANGE_SETUP_SKIP)
I'm not sure about replacing STATUS_BAR_SUBRANGE_SETUP_NOSPC by
STATUS_BAR_SUBRANGE_SETUP_SKIP
The selftest will use if (ret == -ENOSPC), so we need to return this
information (bellow). and semantically SKIP does not imply ENOPSC
(instead of the contrary)
as you prefer,
>
> Personally, I'm not a big fan of having a common function for both
> pci_endpoint_test_bar_subrange_setup() and
> pci_endpoint_test_bar_subrange_clear() and then sending in parameters
> for which bits to check. It make it hard to have small differences
> between them (like checking for an bit that is only valid for one of
> the commands).
>
> I can understand why Koichiro wrote it why he did, but since we now
> want differences, perhaps add a preparation patch that makes it two
> separate functions?
>
> It is not like pci_endpoint_test_bar_subrange_cmd() is a big function
> anyway.
>
To minimize changes, what about something like:
unsigned int fail_bit = status & fail_bits;
if (fail_bit == STATUS_BAR_SUBRANGE_SETUP_NOSPC)
return -ENOSPC;
if (fail_bit == STATUS_BAR_SUBRANGE_SETUP_FAIL)
return -EIO;
called with
pci_endpoint_test_bar_subrange_cmd(...,
STATUS_BAR_SUBRANGE_SETUP_FAIL | STATUS_BAR_SUBRANGE_SETUP_NOSPC);
replacing the fail_bit parameter with fail_bits.
Thus we don't need to specialize the test with 'command', But I prefer
to let Koichiro refactor the function if it is better
thank you,
Christian
>
> Kind regards,
> Niklas
On Fri, Mar 20, 2026 at 10:35:20AM +0100, Christian Bruel wrote:
> Hello,
>
> > >
> > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> > > index 55e128ed82f00ae13b6fe9768cdbe56adbe8f9da..34ba06fb53f04e48c1c05f4aae85e6ecd03ef447 100644
> > > --- a/drivers/misc/pci_endpoint_test.c
> > > +++ b/drivers/misc/pci_endpoint_test.c
> > > @@ -61,6 +61,7 @@
> > > #define STATUS_BAR_SUBRANGE_SETUP_FAIL BIT(15)
> > > #define STATUS_BAR_SUBRANGE_CLEAR_SUCCESS BIT(16)
> > > #define STATUS_BAR_SUBRANGE_CLEAR_FAIL BIT(17)
> > > +#define STATUS_BAR_SUBRANGE_SETUP_NOSPC BIT(18)
> > > #define PCI_ENDPOINT_TEST_LOWER_SRC_ADDR 0x0c
> > > #define PCI_ENDPOINT_TEST_UPPER_SRC_ADDR 0x10
> > > @@ -476,8 +477,11 @@ static int pci_endpoint_test_bar_subrange_cmd(struct pci_endpoint_test *test,
> > > return -ETIMEDOUT;
> > > status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
> > > - if (status & fail_bit)
> > > + if (status & fail_bit) {
> > > + if (status & STATUS_BAR_SUBRANGE_SETUP_NOSPC)
> > > + return -ENOSPC;
> >
> > Perhaps this should be something like:
> >
> > if (command == COMMAND_BAR_SUBRANGE_SETUP && status & STATUS_BAR_SUBRANGE_SETUP_SKIP)
>
> I'm not sure about replacing STATUS_BAR_SUBRANGE_SETUP_NOSPC by
> STATUS_BAR_SUBRANGE_SETUP_SKIP
>
> The selftest will use if (ret == -ENOSPC), so we need to return this
> information (bellow). and semantically SKIP does not imply ENOPSC (instead
> of the contrary)
>
> as you prefer,
FWIW, I think an even better solution is to introduce a new register,
named e.g. errno in struct pci_epf_test_reg;
That way, we don't need to take a new bit, e.g.:
+#define STATUS_BAR_SUBRANGE_SETUP_NOSPC BIT(18)
For every unique error code a command can return.
We would simply return STATUS_BAR_SUBRANGE_CLEAR_FAIL, and then the host
side driver looks at the errno register to see the specific error code.
This way, all other _FAIL commands could also return a more specific error
in case of failure.
>
> To minimize changes, what about something like:
>
> unsigned int fail_bit = status & fail_bits;
>
> if (fail_bit == STATUS_BAR_SUBRANGE_SETUP_NOSPC)
> return -ENOSPC;
>
> if (fail_bit == STATUS_BAR_SUBRANGE_SETUP_FAIL)
> return -EIO;
>
> called with
>
> pci_endpoint_test_bar_subrange_cmd(...,
> STATUS_BAR_SUBRANGE_SETUP_FAIL | STATUS_BAR_SUBRANGE_SETUP_NOSPC);
>
> replacing the fail_bit parameter with fail_bits.
Sounds okay to me, but I think the idea of adding an errno register is
more extensible.
Kind regards,
Niklas
On 3/20/26 12:16, Niklas Cassel wrote:
> On Fri, Mar 20, 2026 at 10:35:20AM +0100, Christian Bruel wrote:
>> Hello,
>>
>>>>
>>>> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
>>>> index 55e128ed82f00ae13b6fe9768cdbe56adbe8f9da..34ba06fb53f04e48c1c05f4aae85e6ecd03ef447 100644
>>>> --- a/drivers/misc/pci_endpoint_test.c
>>>> +++ b/drivers/misc/pci_endpoint_test.c
>>>> @@ -61,6 +61,7 @@
>>>> #define STATUS_BAR_SUBRANGE_SETUP_FAIL BIT(15)
>>>> #define STATUS_BAR_SUBRANGE_CLEAR_SUCCESS BIT(16)
>>>> #define STATUS_BAR_SUBRANGE_CLEAR_FAIL BIT(17)
>>>> +#define STATUS_BAR_SUBRANGE_SETUP_NOSPC BIT(18)
>>>> #define PCI_ENDPOINT_TEST_LOWER_SRC_ADDR 0x0c
>>>> #define PCI_ENDPOINT_TEST_UPPER_SRC_ADDR 0x10
>>>> @@ -476,8 +477,11 @@ static int pci_endpoint_test_bar_subrange_cmd(struct pci_endpoint_test *test,
>>>> return -ETIMEDOUT;
>>>> status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
>>>> - if (status & fail_bit)
>>>> + if (status & fail_bit) {
>>>> + if (status & STATUS_BAR_SUBRANGE_SETUP_NOSPC)
>>>> + return -ENOSPC;
>>>
>>> Perhaps this should be something like:
>>>
>>> if (command == COMMAND_BAR_SUBRANGE_SETUP && status & STATUS_BAR_SUBRANGE_SETUP_SKIP)
>>
>> I'm not sure about replacing STATUS_BAR_SUBRANGE_SETUP_NOSPC by
>> STATUS_BAR_SUBRANGE_SETUP_SKIP
>>
>> The selftest will use if (ret == -ENOSPC), so we need to return this
>> information (bellow). and semantically SKIP does not imply ENOPSC (instead
>> of the contrary)
>>
>> as you prefer,
>
> FWIW, I think an even better solution is to introduce a new register,
> named e.g. errno in struct pci_epf_test_reg;
>
> That way, we don't need to take a new bit, e.g.:
> +#define STATUS_BAR_SUBRANGE_SETUP_NOSPC BIT(18)
>
> For every unique error code a command can return.
>
> We would simply return STATUS_BAR_SUBRANGE_CLEAR_FAIL, and then the host
> side driver looks at the errno register to see the specific error code.
>
> This way, all other _FAIL commands could also return a more specific error
> in case of failure.
>
>
>>
>> To minimize changes, what about something like:
>>
>> unsigned int fail_bit = status & fail_bits;
>>
>> if (fail_bit == STATUS_BAR_SUBRANGE_SETUP_NOSPC)
>> return -ENOSPC;
>>
>> if (fail_bit == STATUS_BAR_SUBRANGE_SETUP_FAIL)
>> return -EIO;
>>
>> called with
>>
>> pci_endpoint_test_bar_subrange_cmd(...,
>> STATUS_BAR_SUBRANGE_SETUP_FAIL | STATUS_BAR_SUBRANGE_SETUP_NOSPC);
>>
>> replacing the fail_bit parameter with fail_bits.
>
> Sounds okay to me, but I think the idea of adding an errno register is
> more extensible.
>
But it is not consistent with other tests that use the status field to
carry error information. For example, pci_epf_test_read/write/copy, set
the STATUS_SRC/DTS_ADDR_INVALID bit to distinguish these errors from
other -EINVAL.
Introducing a new errno field would result in having two different APIs
for returning failure status.
In light of the STATUS_SRC/DST_ADDR_INVALID usage, I wonder if my
initial proposal to set a status bit along with the FAIL bit might
actually be simpler and more consistent what is already done here.
thank you,
Christian
>
> Kind regards,
> Niklas
On Fri, Mar 20, 2026 at 02:25:10PM +0100, Christian Bruel wrote: > On 3/20/26 12:16, Niklas Cassel wrote: > > > > FWIW, I think an even better solution is to introduce a new register, > > named e.g. errno in struct pci_epf_test_reg; > > > > That way, we don't need to take a new bit, e.g.: > > +#define STATUS_BAR_SUBRANGE_SETUP_NOSPC BIT(18) > > > > For every unique error code a command can return. > > > > We would simply return STATUS_BAR_SUBRANGE_CLEAR_FAIL, and then the host > > side driver looks at the errno register to see the specific error code. > > > > This way, all other _FAIL commands could also return a more specific error > > in case of failure. (snip) > But it is not consistent with other tests that use the status field to carry > error information. For example, pci_epf_test_read/write/copy, set the > STATUS_SRC/DTS_ADDR_INVALID bit to distinguish these errors from other > -EINVAL. > > Introducing a new errno field would result in having two different APIs for > returning failure status. > > In light of the STATUS_SRC/DST_ADDR_INVALID usage, I wonder if my initial > proposal to set a status bit along with the FAIL bit might actually be > simpler and more consistent what is already done here. You are right that the read/write/copy tests actually set two bits in the status register. With: #define STATUS_SRC_ADDR_INVALID BIT(7) #define STATUS_DST_ADDR_INVALID BIT(8) Potentially being reused by the write/copy/read tests. At least these bits are reuse for more than one test case. So, perhaps to keep things in line with the existing design perhaps just name the new bit something like: #define STATUS_SKIP_INSUFFICIENT_RESOURCES BIT(18) or something like that. At least then, other test cases will be able to reuse this status bit, just like multiple test cases can use the STATUS_SRC_ADDR_INVALID status bits. And yes, I agree that, like you do in your initial proposal, having this new STATUS_SKIP_INSUFFICIENT_RESOURCES bit set in addition to the STATUS_BAR_SUBRANGE_CLEAR_FAIL bit, is probably the best way forward that is in line with the current design. Kind regards, Niklas
© 2016 - 2026 Red Hat, Inc.