diff mbox

xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()

Message ID 20140929140229.GA28279@laptop.dumpdata.com (mailing list archive)
State New, archived
Headers show

Commit Message

Konrad Rzeszutek Wilk Sept. 29, 2014, 2:02 p.m. UTC
On Sat, Sep 27, 2014 at 12:36:42AM +0800, Chen Gang wrote:
> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()

Only on the first depth, not on the subsequent ones (as in if
the first xenbus_switch_fail fails, it won't try to call
xenbus_switch_state again and again).

> internally, so need not return any status value, then use 'void' instead
> of 'int' for xenbus_switch_state() and __xenbus_switch_state().

When that switch occurs (to XenbusStateConnected) won't the watches
fire - meaning we MUST make sure that the watch functions - if they
use the xenbus_switch_state() they MUST not hold any locks - because
they could be executed once more?

Oh wait, we don't have to worry about that right now as the callbacks
that pick up the messages from the XenBus are all gated on one mutex
anyhow.

Hm, anyhow, I would add this extra piece of information to the patch:


> 
> Also need be sure that all callers which check the return value must let
> 'err' be 0.

I am bit uncomfortable with that, that is due to:


.. snip..
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index 9c47b89..b5c3d47 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
>  	if (err)
>  		pr_debug("Error writing multi-queue-max-queues\n");
>  
> -	err = xenbus_switch_state(dev, XenbusStateInitWait);
> -	if (err)
> -		goto fail;
> -
> +	xenbus_switch_state(dev, XenbusStateInitWait);

Which if it fails it won't call:

354 fail:                                                                           
355         pr_debug("failed\n");                                                   
356         netback_remove(dev);                                                    
357         return err;         


And since there is no watch on the backend state to go in Closing it won't
ever call those and we leak memory.

The same is for xen-blkback mechanism in the probe function.

>  	be->state = XenbusStateInitWait;
>  
>  	/* This kicks hotplug scripts, so do it immediately. */
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index 53df39a..c1d73b7 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -901,18 +901,16 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
>  		}
>  	}
>  
> -	err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> -
> +	xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> +	return 0;
>  out:
>  	return err;
>  }
>  
>  static int pcifront_try_disconnect(struct pcifront_device *pdev)
>  {
> -	int err = 0;
>  	enum xenbus_state prev_state;
>  
> -
>  	prev_state = xenbus_read_driver_state(pdev->xdev->nodename);
>  
>  	if (prev_state >= XenbusStateClosing)
> @@ -923,11 +921,10 @@ static int pcifront_try_disconnect(struct pcifront_device *pdev)
>  		pcifront_disconnect(pdev);
>  	}
>  
> -	err = xenbus_switch_state(pdev->xdev, XenbusStateClosed);
> +	xenbus_switch_state(pdev->xdev, XenbusStateClosed);
>  
>  out:
> -
> -	return err;
> +	return 0;
>  }
>  
>  static int pcifront_attach_devices(struct pcifront_device *pdev)
> @@ -1060,8 +1057,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
>  			domain, bus, slot, func);
>  	}
>  
> -	err = xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> -
> +	xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> +	return 0;
>  out:
>  	return err;
>  }
> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> index c214daa..630a215 100644

The xen-pcifront are OK, this one:

> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -184,10 +184,7 @@ static int xen_pcibk_attach(struct xen_pcibk_device *pdev)
>  
>  	dev_dbg(&pdev->xdev->dev, "Connecting...\n");
>  
> -	err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> -	if (err)
> -		xenbus_dev_fatal(pdev->xdev, err,
> -				 "Error switching to connected state!");
> +	xenbus_switch_state(pdev->xdev, XenbusStateConnected);


This one is OK
>  
>  	dev_dbg(&pdev->xdev->dev, "Connected? %d\n", err);
>  out:
> @@ -500,12 +497,7 @@ static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev)
>  		}
>  	}
>  
> -	err = xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
> -	if (err) {
> -		xenbus_dev_fatal(pdev->xdev, err,
> -				 "Error switching to reconfigured state!");
> -		goto out;
> -	}
> +	xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);

That is OKish, thought I am not too happy about us holding the lock.
>  
>  out:
>  	mutex_unlock(&pdev->dev_lock);
> @@ -640,10 +632,7 @@ static int xen_pcibk_setup_backend(struct xen_pcibk_device *pdev)
>  		goto out;
>  	}
>  
> -	err = xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
> -	if (err)
> -		xenbus_dev_fatal(pdev->xdev, err,
> -				 "Error switching to initialised state!");
> +	xenbus_switch_state(pdev->xdev, XenbusStateInitialised);

And this one needs that comment above about the mutex.
>  
>  out:
>  	mutex_unlock(&pdev->dev_lock);
> @@ -683,9 +672,7 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
>  	}
>  
>  	/* wait for xend to configure us */
> -	err = xenbus_switch_state(dev, XenbusStateInitWait);
> -	if (err)
> -		goto out;
> +	xenbus_switch_state(dev, XenbusStateInitWait);

That means the error that is returned on 'probe' is always zero. I don't
think that is right as we did fail to establish a connection.

>  
>  	/* watch the backend node for backend configuration information */
>  	err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch,
> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
> index ad11258..847bc9c 100644
> --- a/drivers/xen/xen-scsiback.c
> +++ b/drivers/xen/xen-scsiback.c
> @@ -1225,10 +1225,7 @@ static int scsiback_probe(struct xenbus_device *dev,
>  	if (err)
>  		xenbus_dev_error(dev, err, "writing feature-sg-grant");
>  
> -	err = xenbus_switch_state(dev, XenbusStateInitWait);
> -	if (err)
> -		goto fail;
> -
> +	xenbus_switch_state(dev, XenbusStateInitWait);
>  	return 0;
>  
>  fail:
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Vrabel Sept. 29, 2014, 2:17 p.m. UTC | #1
On 29/09/14 15:02, Konrad Rzeszutek Wilk wrote:
> On Sat, Sep 27, 2014 at 12:36:42AM +0800, Chen Gang wrote:
>> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
> 
> Only on the first depth, not on the subsequent ones (as in if
> the first xenbus_switch_fail fails, it won't try to call
> xenbus_switch_state again and again).
> 
>> internally, so need not return any status value, then use 'void' instead
>> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
> 
> When that switch occurs (to XenbusStateConnected) won't the watches
> fire - meaning we MUST make sure that the watch functions - if they
> use the xenbus_switch_state() they MUST not hold any locks - because
> they could be executed once more?
> 
> Oh wait, we don't have to worry about that right now as the callbacks
> that pick up the messages from the XenBus are all gated on one mutex
> anyhow.
> 
> Hm, anyhow, I would add this extra piece of information to the patch:
> 
> 
> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> index c214daa..f7399fd 100644
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -661,6 +661,12 @@ static void xen_pcibk_be_watch(struct xenbus_watch *watch,
>  
>  	switch (xenbus_read_driver_state(pdev->xdev->nodename)) {
>  	case XenbusStateInitWait:
> +		/*
> +		 * xenbus_switch_state can call xenbus_switch_fatal which will
> +		 * immediately set the state to XenbusStateClosing which
> +		 * means if we were reading for it here we MUST drop any
> +		 * locks so that we don't dead-lock.
> +		 */

Watches are asynchronous and serialised by the xenwatch thread.  I can't
see what deadlock you're talking about here.  Particularly since the
backend doesn't watch its own state node (it watches the frontend one).

>  		xen_pcibk_setup_backend(pdev);
>  		break;
>  
>>
>> Also need be sure that all callers which check the return value must let
>> 'err' be 0.
> 
> I am bit uncomfortable with that, that is due to:
> 
> 
> .. snip..
>> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
>> index 9c47b89..b5c3d47 100644
>> --- a/drivers/net/xen-netback/xenbus.c
>> +++ b/drivers/net/xen-netback/xenbus.c
>> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
>>  	if (err)
>>  		pr_debug("Error writing multi-queue-max-queues\n");
>>  
>> -	err = xenbus_switch_state(dev, XenbusStateInitWait);
>> -	if (err)
>> -		goto fail;
>> -
>> +	xenbus_switch_state(dev, XenbusStateInitWait);
> 
> Which if it fails it won't call:
> 
> 354 fail:                                                                           
> 355         pr_debug("failed\n");                                                   
> 356         netback_remove(dev);                                                    
> 357         return err;         
> 
> 
> And since there is no watch on the backend state to go in Closing it won't
> ever call those and we leak memory.

It's not leaking the memory.  All resources will be recovered when the
device is removed.

> The same is for xen-blkback mechanism in the probe function.

David

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Konrad Rzeszutek Wilk Sept. 29, 2014, 3:40 p.m. UTC | #2
On Mon, Sep 29, 2014 at 03:17:10PM +0100, David Vrabel wrote:
> On 29/09/14 15:02, Konrad Rzeszutek Wilk wrote:
> > On Sat, Sep 27, 2014 at 12:36:42AM +0800, Chen Gang wrote:
> >> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
> > 
> > Only on the first depth, not on the subsequent ones (as in if
> > the first xenbus_switch_fail fails, it won't try to call
> > xenbus_switch_state again and again).
> > 
> >> internally, so need not return any status value, then use 'void' instead
> >> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
> > 
> > When that switch occurs (to XenbusStateConnected) won't the watches
> > fire - meaning we MUST make sure that the watch functions - if they
> > use the xenbus_switch_state() they MUST not hold any locks - because
> > they could be executed once more?
> > 
> > Oh wait, we don't have to worry about that right now as the callbacks
> > that pick up the messages from the XenBus are all gated on one mutex
> > anyhow.
> > 
> > Hm, anyhow, I would add this extra piece of information to the patch:
> > 
> > 
> > diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> > index c214daa..f7399fd 100644
> > --- a/drivers/xen/xen-pciback/xenbus.c
> > +++ b/drivers/xen/xen-pciback/xenbus.c
> > @@ -661,6 +661,12 @@ static void xen_pcibk_be_watch(struct xenbus_watch *watch,
> >  
> >  	switch (xenbus_read_driver_state(pdev->xdev->nodename)) {
> >  	case XenbusStateInitWait:
> > +		/*
> > +		 * xenbus_switch_state can call xenbus_switch_fatal which will
> > +		 * immediately set the state to XenbusStateClosing which
> > +		 * means if we were reading for it here we MUST drop any
> > +		 * locks so that we don't dead-lock.
> > +		 */
> 
> Watches are asynchronous and serialised by the xenwatch thread.  I can't
> see what deadlock you're talking about here.  Particularly since the
> backend doesn't watch its own state node (it watches the frontend one).
> 
> >  		xen_pcibk_setup_backend(pdev);
> >  		break;
> >  
> >>
> >> Also need be sure that all callers which check the return value must let
> >> 'err' be 0.
> > 
> > I am bit uncomfortable with that, that is due to:
> > 
> > 
> > .. snip..
> >> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> >> index 9c47b89..b5c3d47 100644
> >> --- a/drivers/net/xen-netback/xenbus.c
> >> +++ b/drivers/net/xen-netback/xenbus.c
> >> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
> >>  	if (err)
> >>  		pr_debug("Error writing multi-queue-max-queues\n");
> >>  
> >> -	err = xenbus_switch_state(dev, XenbusStateInitWait);
> >> -	if (err)
> >> -		goto fail;
> >> -
> >> +	xenbus_switch_state(dev, XenbusStateInitWait);
> > 
> > Which if it fails it won't call:
> > 
> > 354 fail:                                                                           
> > 355         pr_debug("failed\n");                                                   
> > 356         netback_remove(dev);                                                    
> > 357         return err;         
> > 
> > 
> > And since there is no watch on the backend state to go in Closing it won't
> > ever call those and we leak memory.
> 
> It's not leaking the memory.  All resources will be recovered when the
> device is removed.

I presume you mean when the XenBus entries are torn down? It does look
like it would call the .remove functionality. That should take care of that.

In which case we can just remove all of the 'netback_remove()' and also
remove some of the labels.


> 
> > The same is for xen-blkback mechanism in the probe function.
> 
> David
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Vrabel Sept. 29, 2014, 5:14 p.m. UTC | #3
On 29/09/14 16:40, Konrad Rzeszutek Wilk wrote:
> On Mon, Sep 29, 2014 at 03:17:10PM +0100, David Vrabel wrote:
>> On 29/09/14 15:02, Konrad Rzeszutek Wilk wrote:
>>> On Sat, Sep 27, 2014 at 12:36:42AM +0800, Chen Gang wrote:
>>>> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
>>>
>>> Only on the first depth, not on the subsequent ones (as in if
>>> the first xenbus_switch_fail fails, it won't try to call
>>> xenbus_switch_state again and again).
>>>
>>>> internally, so need not return any status value, then use 'void' instead
>>>> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
>>>
>>> When that switch occurs (to XenbusStateConnected) won't the watches
>>> fire - meaning we MUST make sure that the watch functions - if they
>>> use the xenbus_switch_state() they MUST not hold any locks - because
>>> they could be executed once more?
>>>
>>> Oh wait, we don't have to worry about that right now as the callbacks
>>> that pick up the messages from the XenBus are all gated on one mutex
>>> anyhow.
>>>
>>> Hm, anyhow, I would add this extra piece of information to the patch:
>>>
>>>
>>> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
>>> index c214daa..f7399fd 100644
>>> --- a/drivers/xen/xen-pciback/xenbus.c
>>> +++ b/drivers/xen/xen-pciback/xenbus.c
>>> @@ -661,6 +661,12 @@ static void xen_pcibk_be_watch(struct xenbus_watch *watch,
>>>  
>>>  	switch (xenbus_read_driver_state(pdev->xdev->nodename)) {
>>>  	case XenbusStateInitWait:
>>> +		/*
>>> +		 * xenbus_switch_state can call xenbus_switch_fatal which will
>>> +		 * immediately set the state to XenbusStateClosing which
>>> +		 * means if we were reading for it here we MUST drop any
>>> +		 * locks so that we don't dead-lock.
>>> +		 */
>>
>> Watches are asynchronous and serialised by the xenwatch thread.  I can't
>> see what deadlock you're talking about here.  Particularly since the
>> backend doesn't watch its own state node (it watches the frontend one).
>>
>>>  		xen_pcibk_setup_backend(pdev);
>>>  		break;
>>>  
>>>>
>>>> Also need be sure that all callers which check the return value must let
>>>> 'err' be 0.
>>>
>>> I am bit uncomfortable with that, that is due to:
>>>
>>>
>>> .. snip..
>>>> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
>>>> index 9c47b89..b5c3d47 100644
>>>> --- a/drivers/net/xen-netback/xenbus.c
>>>> +++ b/drivers/net/xen-netback/xenbus.c
>>>> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
>>>>  	if (err)
>>>>  		pr_debug("Error writing multi-queue-max-queues\n");
>>>>  
>>>> -	err = xenbus_switch_state(dev, XenbusStateInitWait);
>>>> -	if (err)
>>>> -		goto fail;
>>>> -
>>>> +	xenbus_switch_state(dev, XenbusStateInitWait);
>>>
>>> Which if it fails it won't call:
>>>
>>> 354 fail:                                                                           
>>> 355         pr_debug("failed\n");                                                   
>>> 356         netback_remove(dev);                                                    
>>> 357         return err;         
>>>
>>>
>>> And since there is no watch on the backend state to go in Closing it won't
>>> ever call those and we leak memory.
>>
>> It's not leaking the memory.  All resources will be recovered when the
>> device is removed.
> 
> I presume you mean when the XenBus entries are torn down? It does look
> like it would call the .remove functionality. That should take care of that.
> 
> In which case we can just remove all of the 'netback_remove()' and also
> remove some of the labels.

No.  If the final xenbus_switch_state() fails then at least the device
is in a consistent state, waiting for the other end to notice.

We don't want to return success from a probe with a half-setup device.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen Gang Sept. 30, 2014, 8:04 a.m. UTC | #4
On 9/29/14 22:02, Konrad Rzeszutek Wilk wrote:
> On Sat, Sep 27, 2014 at 12:36:42AM +0800, Chen Gang wrote:
>> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
> 
> Only on the first depth, not on the subsequent ones (as in if
> the first xenbus_switch_fail fails, it won't try to call
> xenbus_switch_state again and again).
> 

Yeah, I guess you want to give more completion for this comment, do not
mean the original comments is incorrect.

[...]
>>
>> Also need be sure that all callers which check the return value must let
>> 'err' be 0.
> 
> I am bit uncomfortable with that, that is due to:
> 
> 
> .. snip..
>> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
>> index 9c47b89..b5c3d47 100644
>> --- a/drivers/net/xen-netback/xenbus.c
>> +++ b/drivers/net/xen-netback/xenbus.c
>> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
>>  	if (err)
>>  		pr_debug("Error writing multi-queue-max-queues\n");
>>  
>> -	err = xenbus_switch_state(dev, XenbusStateInitWait);
>> -	if (err)
>> -		goto fail;
>> -
>> +	xenbus_switch_state(dev, XenbusStateInitWait);
> 
> Which if it fails it won't call:
> 
> 354 fail:                                                                           
> 355         pr_debug("failed\n");                                                   
> 356         netback_remove(dev);                                                    
> 357         return err;         
> 

Originally, I intended left 'fail' code block for the next patch (which
I thought the next patch would need it).

Hmm... but any way, originally, I really need give additional comments
for it.

[...]

I skip all other contents which have already discussed by maintainers,
if I really miss something, please let me know.


Thanks.
diff mbox

Patch

diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index c214daa..f7399fd 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -661,6 +661,12 @@  static void xen_pcibk_be_watch(struct xenbus_watch *watch,
 
 	switch (xenbus_read_driver_state(pdev->xdev->nodename)) {
 	case XenbusStateInitWait:
+		/*
+		 * xenbus_switch_state can call xenbus_switch_fatal which will
+		 * immediately set the state to XenbusStateClosing which
+		 * means if we were reading for it here we MUST drop any
+		 * locks so that we don't dead-lock.
+		 */
 		xen_pcibk_setup_backend(pdev);
 		break;