2014-10-06 02:59:01

by Chen Gang

[permalink] [raw]
Subject: [PATCH next] xen: pcifront: Process failure for pcifront_(re)scan_root()

When pcifront_rescan_root() or pcifront_scan_root() fails, need return
error code, neither set XenbusStateConnected state, just like the other
areas have done.

For pcifront_rescan_root(), it will return error code ("num_roots = 0;",
skip xenbus_switch_state return value).

For pcifront_scan_root(), it will return 0 ("num_roots = 0;", set 0 by
the return value of xenbus_switch_state, which always return 0, at
present).

Signed-off-by: Chen Gang <[email protected]>
---
drivers/pci/xen-pcifront.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index 53df39a..d78d884 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -866,6 +866,11 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
xenbus_dev_error(pdev->xdev, err,
"No PCI Roots found, trying 0000:00");
err = pcifront_scan_root(pdev, 0, 0);
+ if (err) {
+ xenbus_dev_fatal(pdev->xdev, err,
+ "Error scanning PCI root 0000:00");
+ goto out;
+ }
num_roots = 0;
} else if (err != 1) {
if (err == 0)
@@ -947,6 +952,11 @@ static int pcifront_attach_devices(struct pcifront_device *pdev)
xenbus_dev_error(pdev->xdev, err,
"No PCI Roots found, trying 0000:00");
err = pcifront_rescan_root(pdev, 0, 0);
+ if (err) {
+ xenbus_dev_fatal(pdev->xdev, err,
+ "Error scanning PCI root 0000:00");
+ goto out;
+ }
num_roots = 0;
} else if (err != 1) {
if (err == 0)
--
1.9.3


2014-10-15 00:09:17

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [PATCH next] xen: pcifront: Process failure for pcifront_(re)scan_root()

On Mon, Oct 06, 2014 at 11:04:45AM +0800, Chen Gang wrote:
> When pcifront_rescan_root() or pcifront_scan_root() fails, need return
> error code, neither set XenbusStateConnected state, just like the other
> areas have done.
>
> For pcifront_rescan_root(), it will return error code ("num_roots = 0;",
> skip xenbus_switch_state return value).
>
> For pcifront_scan_root(), it will return 0 ("num_roots = 0;", set 0 by
> the return value of xenbus_switch_state, which always return 0, at
> present).

The changelog is somewhat confusing because it talks about the patch hunks
in reverse order (the pcifront_scan_root() change is first in the patch,
but the changelog mentions pcifront_rescan_root() first). I *think* this
means:

When pcifront_try_connect() finds no PCI roots, it falls back to calling
pcifront_scan_root() for 0000:00. If that fails, it used to switch to
XenbusStateConnected and return success (because xenbus_switch_state()
currently always succeeds).

If pcifront_scan_root() fails, leave the XenbusState unchanged and
return an error code.

Similarly, pcifront_attach_devices() falls back to calling
pcifront_rescan_root() for 0000:00. If that fails, it used to
switch to XenbusStateConnected and return an error code.

If pcifront_rescan_root() fails, leave the XenbusState unchanged and
return the error code.

The "num_roots" part doesn't seem relevant to me.

> Signed-off-by: Chen Gang <[email protected]>

Konrad, if you want to take this, feel free. Otherwise, if you ack it and
you think my changelog understanding makes sense, I can pick it up.

It does seem odd that pcifront_attach_devices() ignores the
xenbus_switch_state() return value while pcifront_try_connect() does not.
But many other callers also ignore the return value, so maybe that's OK.

Bjorn

> ---
> drivers/pci/xen-pcifront.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index 53df39a..d78d884 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -866,6 +866,11 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
> xenbus_dev_error(pdev->xdev, err,
> "No PCI Roots found, trying 0000:00");
> err = pcifront_scan_root(pdev, 0, 0);
> + if (err) {
> + xenbus_dev_fatal(pdev->xdev, err,
> + "Error scanning PCI root 0000:00");
> + goto out;
> + }
> num_roots = 0;
> } else if (err != 1) {
> if (err == 0)
> @@ -947,6 +952,11 @@ static int pcifront_attach_devices(struct pcifront_device *pdev)
> xenbus_dev_error(pdev->xdev, err,
> "No PCI Roots found, trying 0000:00");
> err = pcifront_rescan_root(pdev, 0, 0);
> + if (err) {
> + xenbus_dev_fatal(pdev->xdev, err,
> + "Error scanning PCI root 0000:00");
> + goto out;
> + }
> num_roots = 0;
> } else if (err != 1) {
> if (err == 0)
> --
> 1.9.3

2014-10-15 00:20:17

by Chen Gang

[permalink] [raw]
Subject: Re: [PATCH next] xen: pcifront: Process failure for pcifront_(re)scan_root()


At least for me, what you said sound OK.

Thanks.


Send from Lenovo A788t.

Bjorn Helgaas <[email protected]> wrote:

>On Mon, Oct 06, 2014 at 11:04:45AM +0800, Chen Gang wrote:
>> When pcifront_rescan_root() or pcifront_scan_root() fails, need return
>> error code, neither set XenbusStateConnected state, just like the other
>> areas have done.
>>
>> For pcifront_rescan_root(), it will return error code ("num_roots = 0;",
>> skip xenbus_switch_state return value).
>>
>> For pcifront_scan_root(), it will return 0 ("num_roots = 0;", set 0 by
>> the return value of xenbus_switch_state, which always return 0, at
>> present).
>
>The changelog is somewhat confusing because it talks about the patch hunks
>in reverse order (the pcifront_scan_root() change is first in the patch,
>but the changelog mentions pcifront_rescan_root() first). I *think* this
>means:
>
> When pcifront_try_connect() finds no PCI roots, it falls back to calling
> pcifront_scan_root() for 0000:00. If that fails, it used to switch to
> XenbusStateConnected and return success (because xenbus_switch_state()
> currently always succeeds).
>
> If pcifront_scan_root() fails, leave the XenbusState unchanged and
> return an error code.
>
> Similarly, pcifront_attach_devices() falls back to calling
> pcifront_rescan_root() for 0000:00. If that fails, it used to
> switch to XenbusStateConnected and return an error code.
>
> If pcifront_rescan_root() fails, leave the XenbusState unchanged and
> return the error code.
>
>The "num_roots" part doesn't seem relevant to me.
>
>> Signed-off-by: Chen Gang <[email protected]>
>
>Konrad, if you want to take this, feel free. Otherwise, if you ack it and
>you think my changelog understanding makes sense, I can pick it up.
>
>It does seem odd that pcifront_attach_devices() ignores the
>xenbus_switch_state() return value while pcifront_try_connect() does not.
>But many other callers also ignore the return value, so maybe that's OK.
>
>Bjorn
>
>> ---
>> drivers/pci/xen-pcifront.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
>> index 53df39a..d78d884 100644
>> --- a/drivers/pci/xen-pcifront.c
>> +++ b/drivers/pci/xen-pcifront.c
>> @@ -866,6 +866,11 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
>> xenbus_dev_error(pdev->xdev, err,
>> "No PCI Roots found, trying 0000:00");
>> err = pcifront_scan_root(pdev, 0, 0);
>> + if (err) {
>> + xenbus_dev_fatal(pdev->xdev, err,
>> + "Error scanning PCI root 0000:00");
>> + goto out;
>> + }
>> num_roots = 0;
>> } else if (err != 1) {
>> if (err == 0)
>> @@ -947,6 +952,11 @@ static int pcifront_attach_devices(struct pcifront_device *pdev)
>> xenbus_dev_error(pdev->xdev, err,
>> "No PCI Roots found, trying 0000:00");
>> err = pcifront_rescan_root(pdev, 0, 0);
>> + if (err) {
>> + xenbus_dev_fatal(pdev->xdev, err,
>> + "Error scanning PCI root 0000:00");
>> + goto out;
>> + }
>> num_roots = 0;
>> } else if (err != 1) {
>> if (err == 0)
>> --
>> 1.9.3
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2014-10-15 21:04:09

by Konrad Rzeszutek Wilk

[permalink] [raw]
Subject: Re: [PATCH next] xen: pcifront: Process failure for pcifront_(re)scan_root()

On Wed, Oct 15, 2014 at 08:20:06AM +0800, Chen Gang wrote:
>
> At least for me, what you said sound OK.

Let me review it - next week.
>
> Thanks.
>
>
> Send from Lenovo A788t.
>
> Bjorn Helgaas <[email protected]> wrote:
>
> >On Mon, Oct 06, 2014 at 11:04:45AM +0800, Chen Gang wrote:
> >> When pcifront_rescan_root() or pcifront_scan_root() fails, need return
> >> error code, neither set XenbusStateConnected state, just like the other
> >> areas have done.
> >>
> >> For pcifront_rescan_root(), it will return error code ("num_roots = 0;",
> >> skip xenbus_switch_state return value).
> >>
> >> For pcifront_scan_root(), it will return 0 ("num_roots = 0;", set 0 by
> >> the return value of xenbus_switch_state, which always return 0, at
> >> present).
> >
> >The changelog is somewhat confusing because it talks about the patch hunks
> >in reverse order (the pcifront_scan_root() change is first in the patch,
> >but the changelog mentions pcifront_rescan_root() first). I *think* this
> >means:
> >
> > When pcifront_try_connect() finds no PCI roots, it falls back to calling
> > pcifront_scan_root() for 0000:00. If that fails, it used to switch to
> > XenbusStateConnected and return success (because xenbus_switch_state()
> > currently always succeeds).
> >
> > If pcifront_scan_root() fails, leave the XenbusState unchanged and
> > return an error code.
> >
> > Similarly, pcifront_attach_devices() falls back to calling
> > pcifront_rescan_root() for 0000:00. If that fails, it used to
> > switch to XenbusStateConnected and return an error code.
> >
> > If pcifront_rescan_root() fails, leave the XenbusState unchanged and
> > return the error code.
> >
> >The "num_roots" part doesn't seem relevant to me.
> >
> >> Signed-off-by: Chen Gang <[email protected]>
> >
> >Konrad, if you want to take this, feel free. Otherwise, if you ack it and
> >you think my changelog understanding makes sense, I can pick it up.
> >
> >It does seem odd that pcifront_attach_devices() ignores the
> >xenbus_switch_state() return value while pcifront_try_connect() does not.
> >But many other callers also ignore the return value, so maybe that's OK.
> >
> >Bjorn
> >
> >> ---
> >> drivers/pci/xen-pcifront.c | 10 ++++++++++
> >> 1 file changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> >> index 53df39a..d78d884 100644
> >> --- a/drivers/pci/xen-pcifront.c
> >> +++ b/drivers/pci/xen-pcifront.c
> >> @@ -866,6 +866,11 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
> >> xenbus_dev_error(pdev->xdev, err,
> >> "No PCI Roots found, trying 0000:00");
> >> err = pcifront_scan_root(pdev, 0, 0);
> >> + if (err) {
> >> + xenbus_dev_fatal(pdev->xdev, err,
> >> + "Error scanning PCI root 0000:00");
> >> + goto out;
> >> + }
> >> num_roots = 0;
> >> } else if (err != 1) {
> >> if (err == 0)
> >> @@ -947,6 +952,11 @@ static int pcifront_attach_devices(struct pcifront_device *pdev)
> >> xenbus_dev_error(pdev->xdev, err,
> >> "No PCI Roots found, trying 0000:00");
> >> err = pcifront_rescan_root(pdev, 0, 0);
> >> + if (err) {
> >> + xenbus_dev_fatal(pdev->xdev, err,
> >> + "Error scanning PCI root 0000:00");
> >> + goto out;
> >> + }
> >> num_roots = 0;
> >> } else if (err != 1) {
> >> if (err == 0)
> >> --
> >> 1.9.3

2014-10-24 22:44:55

by Chen Gang

[permalink] [raw]
Subject: Re: [PATCH next] xen: pcifront: Process failure for pcifront_(re)scan_root()

On 10/16/14 5:03, Konrad Rzeszutek Wilk wrote:
> On Wed, Oct 15, 2014 at 08:20:06AM +0800, Chen Gang wrote:
>>
>> At least for me, what you said sound OK.
>
> Let me review it - next week.

Please help check this patch, when you have time.

Thanks.

>>
>> Thanks.
>>
>>
>> Send from Lenovo A788t.
>>
>> Bjorn Helgaas <[email protected]> wrote:
>>
>>> On Mon, Oct 06, 2014 at 11:04:45AM +0800, Chen Gang wrote:
>>>> When pcifront_rescan_root() or pcifront_scan_root() fails, need return
>>>> error code, neither set XenbusStateConnected state, just like the other
>>>> areas have done.
>>>>
>>>> For pcifront_rescan_root(), it will return error code ("num_roots = 0;",
>>>> skip xenbus_switch_state return value).
>>>>
>>>> For pcifront_scan_root(), it will return 0 ("num_roots = 0;", set 0 by
>>>> the return value of xenbus_switch_state, which always return 0, at
>>>> present).
>>>
>>> The changelog is somewhat confusing because it talks about the patch hunks
>>> in reverse order (the pcifront_scan_root() change is first in the patch,
>>> but the changelog mentions pcifront_rescan_root() first). I *think* this
>>> means:
>>>
>>> When pcifront_try_connect() finds no PCI roots, it falls back to calling
>>> pcifront_scan_root() for 0000:00. If that fails, it used to switch to
>>> XenbusStateConnected and return success (because xenbus_switch_state()
>>> currently always succeeds).
>>>
>>> If pcifront_scan_root() fails, leave the XenbusState unchanged and
>>> return an error code.
>>>
>>> Similarly, pcifront_attach_devices() falls back to calling
>>> pcifront_rescan_root() for 0000:00. If that fails, it used to
>>> switch to XenbusStateConnected and return an error code.
>>>
>>> If pcifront_rescan_root() fails, leave the XenbusState unchanged and
>>> return the error code.
>>>
>>> The "num_roots" part doesn't seem relevant to me.
>>>
>>>> Signed-off-by: Chen Gang <[email protected]>
>>>
>>> Konrad, if you want to take this, feel free. Otherwise, if you ack it and
>>> you think my changelog understanding makes sense, I can pick it up.
>>>
>>> It does seem odd that pcifront_attach_devices() ignores the
>>> xenbus_switch_state() return value while pcifront_try_connect() does not.
>>> But many other callers also ignore the return value, so maybe that's OK.
>>>
>>> Bjorn
>>>
>>>> ---
>>>> drivers/pci/xen-pcifront.c | 10 ++++++++++
>>>> 1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
>>>> index 53df39a..d78d884 100644
>>>> --- a/drivers/pci/xen-pcifront.c
>>>> +++ b/drivers/pci/xen-pcifront.c
>>>> @@ -866,6 +866,11 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
>>>> xenbus_dev_error(pdev->xdev, err,
>>>> "No PCI Roots found, trying 0000:00");
>>>> err = pcifront_scan_root(pdev, 0, 0);
>>>> + if (err) {
>>>> + xenbus_dev_fatal(pdev->xdev, err,
>>>> + "Error scanning PCI root 0000:00");
>>>> + goto out;
>>>> + }
>>>> num_roots = 0;
>>>> } else if (err != 1) {
>>>> if (err == 0)
>>>> @@ -947,6 +952,11 @@ static int pcifront_attach_devices(struct pcifront_device *pdev)
>>>> xenbus_dev_error(pdev->xdev, err,
>>>> "No PCI Roots found, trying 0000:00");
>>>> err = pcifront_rescan_root(pdev, 0, 0);
>>>> + if (err) {
>>>> + xenbus_dev_fatal(pdev->xdev, err,
>>>> + "Error scanning PCI root 0000:00");
>>>> + goto out;
>>>> + }
>>>> num_roots = 0;
>>>> } else if (err != 1) {
>>>> if (err == 0)
>>>> --
>>>> 1.9.3

--
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

2014-11-05 23:31:41

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [PATCH next] xen: pcifront: Process failure for pcifront_(re)scan_root()

[+cc linux-pci again]

On Fri, Oct 24, 2014 at 4:50 PM, Chen Gang <[email protected]> wrote:
> On 10/16/14 5:03, Konrad Rzeszutek Wilk wrote:
>> On Wed, Oct 15, 2014 at 08:20:06AM +0800, Chen Gang wrote:
>>>
>>> At least for me, what you said sound OK.
>>
>> Let me review it - next week.
>
> Please help check this patch, when you have time.

linux-pci got dropped from the cc list, which makes it harder for me
to track this in patchwork.

But I'm waiting for Konrad to either ack this or just take it
directly. Here's my ack if you want it:

Acked-by: Bjorn Helgas <[email protected]>

>>> Bjorn Helgaas <[email protected]> wrote:
>>>
>>>> On Mon, Oct 06, 2014 at 11:04:45AM +0800, Chen Gang wrote:
>>>>> When pcifront_rescan_root() or pcifront_scan_root() fails, need return
>>>>> error code, neither set XenbusStateConnected state, just like the other
>>>>> areas have done.
>>>>>
>>>>> For pcifront_rescan_root(), it will return error code ("num_roots = 0;",
>>>>> skip xenbus_switch_state return value).
>>>>>
>>>>> For pcifront_scan_root(), it will return 0 ("num_roots = 0;", set 0 by
>>>>> the return value of xenbus_switch_state, which always return 0, at
>>>>> present).
>>>>
>>>> The changelog is somewhat confusing because it talks about the patch hunks
>>>> in reverse order (the pcifront_scan_root() change is first in the patch,
>>>> but the changelog mentions pcifront_rescan_root() first). I *think* this
>>>> means:
>>>>
>>>> When pcifront_try_connect() finds no PCI roots, it falls back to calling
>>>> pcifront_scan_root() for 0000:00. If that fails, it used to switch to
>>>> XenbusStateConnected and return success (because xenbus_switch_state()
>>>> currently always succeeds).
>>>>
>>>> If pcifront_scan_root() fails, leave the XenbusState unchanged and
>>>> return an error code.
>>>>
>>>> Similarly, pcifront_attach_devices() falls back to calling
>>>> pcifront_rescan_root() for 0000:00. If that fails, it used to
>>>> switch to XenbusStateConnected and return an error code.
>>>>
>>>> If pcifront_rescan_root() fails, leave the XenbusState unchanged and
>>>> return the error code.
>>>>
>>>> The "num_roots" part doesn't seem relevant to me.
>>>>
>>>>> Signed-off-by: Chen Gang <[email protected]>
>>>>
>>>> Konrad, if you want to take this, feel free. Otherwise, if you ack it and
>>>> you think my changelog understanding makes sense, I can pick it up.
>>>>
>>>> It does seem odd that pcifront_attach_devices() ignores the
>>>> xenbus_switch_state() return value while pcifront_try_connect() does not.
>>>> But many other callers also ignore the return value, so maybe that's OK.
>>>>
>>>> Bjorn
>>>>
>>>>> ---
>>>>> drivers/pci/xen-pcifront.c | 10 ++++++++++
>>>>> 1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
>>>>> index 53df39a..d78d884 100644
>>>>> --- a/drivers/pci/xen-pcifront.c
>>>>> +++ b/drivers/pci/xen-pcifront.c
>>>>> @@ -866,6 +866,11 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
>>>>> xenbus_dev_error(pdev->xdev, err,
>>>>> "No PCI Roots found, trying 0000:00");
>>>>> err = pcifront_scan_root(pdev, 0, 0);
>>>>> + if (err) {
>>>>> + xenbus_dev_fatal(pdev->xdev, err,
>>>>> + "Error scanning PCI root 0000:00");
>>>>> + goto out;
>>>>> + }
>>>>> num_roots = 0;
>>>>> } else if (err != 1) {
>>>>> if (err == 0)
>>>>> @@ -947,6 +952,11 @@ static int pcifront_attach_devices(struct pcifront_device *pdev)
>>>>> xenbus_dev_error(pdev->xdev, err,
>>>>> "No PCI Roots found, trying 0000:00");
>>>>> err = pcifront_rescan_root(pdev, 0, 0);
>>>>> + if (err) {
>>>>> + xenbus_dev_fatal(pdev->xdev, err,
>>>>> + "Error scanning PCI root 0000:00");
>>>>> + goto out;
>>>>> + }
>>>>> num_roots = 0;
>>>>> } else if (err != 1) {
>>>>> if (err == 0)
>>>>> --
>>>>> 1.9.3
>
> --
> Chen Gang
>
> Open, share, and attitude like air, water, and life which God blessed

2014-11-06 03:09:29

by Konrad Rzeszutek Wilk

[permalink] [raw]
Subject: Re: [PATCH next] xen: pcifront: Process failure for pcifront_(re)scan_root()

On Wed, Nov 05, 2014 at 04:31:17PM -0700, Bjorn Helgaas wrote:
> [+cc linux-pci again]
>
> On Fri, Oct 24, 2014 at 4:50 PM, Chen Gang <[email protected]> wrote:
> > On 10/16/14 5:03, Konrad Rzeszutek Wilk wrote:
> >> On Wed, Oct 15, 2014 at 08:20:06AM +0800, Chen Gang wrote:
> >>>
> >>> At least for me, what you said sound OK.
> >>
> >> Let me review it - next week.
> >
> > Please help check this patch, when you have time.
>
> linux-pci got dropped from the cc list, which makes it harder for me
> to track this in patchwork.
>
> But I'm waiting for Konrad to either ack this or just take it
> directly. Here's my ack if you want it:
>
> Acked-by: Bjorn Helgas <[email protected]>

Reviewed-by: Konrad Rzeszutek Wilk <[email protected]>

Bjorn, if you would like to pick it up that would be good!
>
> >>> Bjorn Helgaas <[email protected]> wrote:
> >>>
> >>>> On Mon, Oct 06, 2014 at 11:04:45AM +0800, Chen Gang wrote:
> >>>>> When pcifront_rescan_root() or pcifront_scan_root() fails, need return
> >>>>> error code, neither set XenbusStateConnected state, just like the other
> >>>>> areas have done.
> >>>>>
> >>>>> For pcifront_rescan_root(), it will return error code ("num_roots = 0;",
> >>>>> skip xenbus_switch_state return value).
> >>>>>
> >>>>> For pcifront_scan_root(), it will return 0 ("num_roots = 0;", set 0 by
> >>>>> the return value of xenbus_switch_state, which always return 0, at
> >>>>> present).
> >>>>
> >>>> The changelog is somewhat confusing because it talks about the patch hunks
> >>>> in reverse order (the pcifront_scan_root() change is first in the patch,
> >>>> but the changelog mentions pcifront_rescan_root() first). I *think* this
> >>>> means:
> >>>>
> >>>> When pcifront_try_connect() finds no PCI roots, it falls back to calling
> >>>> pcifront_scan_root() for 0000:00. If that fails, it used to switch to
> >>>> XenbusStateConnected and return success (because xenbus_switch_state()
> >>>> currently always succeeds).
> >>>>
> >>>> If pcifront_scan_root() fails, leave the XenbusState unchanged and
> >>>> return an error code.
> >>>>
> >>>> Similarly, pcifront_attach_devices() falls back to calling
> >>>> pcifront_rescan_root() for 0000:00. If that fails, it used to
> >>>> switch to XenbusStateConnected and return an error code.
> >>>>
> >>>> If pcifront_rescan_root() fails, leave the XenbusState unchanged and
> >>>> return the error code.
> >>>>
> >>>> The "num_roots" part doesn't seem relevant to me.
> >>>>
> >>>>> Signed-off-by: Chen Gang <[email protected]>
> >>>>
> >>>> Konrad, if you want to take this, feel free. Otherwise, if you ack it and
> >>>> you think my changelog understanding makes sense, I can pick it up.
> >>>>
> >>>> It does seem odd that pcifront_attach_devices() ignores the
> >>>> xenbus_switch_state() return value while pcifront_try_connect() does not.
> >>>> But many other callers also ignore the return value, so maybe that's OK.

It is OK. We had an discussion about making the xenbus_switch_state an
void. The reason being that if the state change fails we call xenbus_switch_fatal
(which does not return anything) - which then sets the state to XenbusStateClosing.

But for some drivers it makes sense to know about this failure so that
they can deallocate their resources. So ignoring ret is OK if the driver
is OK handling its deallocation in a different way.

> >>>>
> >>>> Bjorn
> >>>>
> >>>>> ---
> >>>>> drivers/pci/xen-pcifront.c | 10 ++++++++++
> >>>>> 1 file changed, 10 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> >>>>> index 53df39a..d78d884 100644
> >>>>> --- a/drivers/pci/xen-pcifront.c
> >>>>> +++ b/drivers/pci/xen-pcifront.c
> >>>>> @@ -866,6 +866,11 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
> >>>>> xenbus_dev_error(pdev->xdev, err,
> >>>>> "No PCI Roots found, trying 0000:00");
> >>>>> err = pcifront_scan_root(pdev, 0, 0);
> >>>>> + if (err) {
> >>>>> + xenbus_dev_fatal(pdev->xdev, err,
> >>>>> + "Error scanning PCI root 0000:00");
> >>>>> + goto out;
> >>>>> + }
> >>>>> num_roots = 0;
> >>>>> } else if (err != 1) {
> >>>>> if (err == 0)
> >>>>> @@ -947,6 +952,11 @@ static int pcifront_attach_devices(struct pcifront_device *pdev)
> >>>>> xenbus_dev_error(pdev->xdev, err,
> >>>>> "No PCI Roots found, trying 0000:00");
> >>>>> err = pcifront_rescan_root(pdev, 0, 0);
> >>>>> + if (err) {
> >>>>> + xenbus_dev_fatal(pdev->xdev, err,
> >>>>> + "Error scanning PCI root 0000:00");
> >>>>> + goto out;
> >>>>> + }
> >>>>> num_roots = 0;
> >>>>> } else if (err != 1) {
> >>>>> if (err == 0)
> >>>>> --
> >>>>> 1.9.3
> >
> > --
> > Chen Gang
> >
> > Open, share, and attitude like air, water, and life which God blessed

2014-11-06 04:16:58

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [PATCH next] xen: pcifront: Process failure for pcifront_(re)scan_root()

On Mon, Oct 06, 2014 at 11:04:45AM +0800, Chen Gang wrote:
> When pcifront_rescan_root() or pcifront_scan_root() fails, need return
> error code, neither set XenbusStateConnected state, just like the other
> areas have done.
>
> For pcifront_rescan_root(), it will return error code ("num_roots = 0;",
> skip xenbus_switch_state return value).
>
> For pcifront_scan_root(), it will return 0 ("num_roots = 0;", set 0 by
> the return value of xenbus_switch_state, which always return 0, at
> present).
>
> Signed-off-by: Chen Gang <[email protected]>

Applied to pci/virtualization for v3.19, with Konrad's ack and my changelog
rework. Thanks!

> ---
> drivers/pci/xen-pcifront.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index 53df39a..d78d884 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -866,6 +866,11 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
> xenbus_dev_error(pdev->xdev, err,
> "No PCI Roots found, trying 0000:00");
> err = pcifront_scan_root(pdev, 0, 0);
> + if (err) {
> + xenbus_dev_fatal(pdev->xdev, err,
> + "Error scanning PCI root 0000:00");
> + goto out;
> + }
> num_roots = 0;
> } else if (err != 1) {
> if (err == 0)
> @@ -947,6 +952,11 @@ static int pcifront_attach_devices(struct pcifront_device *pdev)
> xenbus_dev_error(pdev->xdev, err,
> "No PCI Roots found, trying 0000:00");
> err = pcifront_rescan_root(pdev, 0, 0);
> + if (err) {
> + xenbus_dev_fatal(pdev->xdev, err,
> + "Error scanning PCI root 0000:00");
> + goto out;
> + }
> num_roots = 0;
> } else if (err != 1) {
> if (err == 0)
> --
> 1.9.3