Re: [Pcihpd-discuss] [PATCH 26/34] PCI: add pci_try_set_mwi

Re: [Pcihpd-discuss] [PATCH 26/34] PCI: add pci_try_set_mwi

[Matthew Wilcox]

On Wed, Jul 11, 2007 at 04:31:40PM -0700, Greg Kroah-Hartman wrote:
> As suggested by Andrew, add pci_try_set_mwi(), which does not require
> return-value checking.

Seems like a daft suggestion.  What's wrong with just removing the
__must_check from pci_set_mwi()?  Did it find any bugs?

> --- a/drivers/scsi/lpfc/lpfc_init.c
> +++ b/drivers/scsi/lpfc/lpfc_init.c
> @@ -1578,10 +1578,7 @@ lpfc_pci_probe_one(struct pci_dev *pdev, const struct pci_device_id *pid)
>  	INIT_LIST_HEAD(&phba->fc_nodes);
>  
>  	pci_set_master(pdev);
> -	retval = pci_set_mwi(pdev);
> -	if (retval)
> -		dev_printk(KERN_WARNING, &pdev->dev,
> -			   "Warning: pci_set_mwi returned %d\n", retval);
> +	pci_try_set_mwi(pdev);

Why remove the warning?  Presumably people want to know if pci_set_mwi
failed.

-- 
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [Pcihpd-discuss] [PATCH 26/34] PCI: add pci_try_set_mwi

[Greg KH]

On Wed, Jul 11, 2007 at 06:01:49PM -0600, Matthew Wilcox wrote:
> On Wed, Jul 11, 2007 at 04:31:40PM -0700, Greg Kroah-Hartman wrote:
> > As suggested by Andrew, add pci_try_set_mwi(), which does not require
> > return-value checking.
> 
> Seems like a daft suggestion.  What's wrong with just removing the
> __must_check from pci_set_mwi()?  Did it find any bugs?
> 
> > --- a/drivers/scsi/lpfc/lpfc_init.c
> > +++ b/drivers/scsi/lpfc/lpfc_init.c
> > @@ -1578,10 +1578,7 @@ lpfc_pci_probe_one(struct pci_dev *pdev, const struct pci_device_id *pid)
> >  	INIT_LIST_HEAD(&phba->fc_nodes);
> >  
> >  	pci_set_master(pdev);
> > -	retval = pci_set_mwi(pdev);
> > -	if (retval)
> > -		dev_printk(KERN_WARNING, &pdev->dev,
> > -			   "Warning: pci_set_mwi returned %d\n", retval);
> > +	pci_try_set_mwi(pdev);
> 
> Why remove the warning?  Presumably people want to know if pci_set_mwi
> failed.

Randy, this was your change, right?

thanks,

greg k-h

Re: [Pcihpd-discuss] [PATCH 26/34] PCI: add pci_try_set_mwi

[Randy Dunlap]

On Wed, 11 Jul 2007 17:37:31 -0700 Greg KH wrote:

> On Wed, Jul 11, 2007 at 06:01:49PM -0600, Matthew Wilcox wrote:
> > On Wed, Jul 11, 2007 at 04:31:40PM -0700, Greg Kroah-Hartman wrote:
> > > As suggested by Andrew, add pci_try_set_mwi(), which does not require
> > > return-value checking.
> > 
> > Seems like a daft suggestion.  What's wrong with just removing the
> > __must_check from pci_set_mwi()?  Did it find any bugs?
> > 
> > > --- a/drivers/scsi/lpfc/lpfc_init.c
> > > +++ b/drivers/scsi/lpfc/lpfc_init.c
> > > @@ -1578,10 +1578,7 @@ lpfc_pci_probe_one(struct pci_dev *pdev, const struct pci_device_id *pid)
> > >  	INIT_LIST_HEAD(&phba->fc_nodes);
> > >  
> > >  	pci_set_master(pdev);
> > > -	retval = pci_set_mwi(pdev);
> > > -	if (retval)
> > > -		dev_printk(KERN_WARNING, &pdev->dev,
> > > -			   "Warning: pci_set_mwi returned %d\n", retval);
> > > +	pci_try_set_mwi(pdev);
> > 
> > Why remove the warning?  Presumably people want to know if pci_set_mwi
> > failed.
> 
> Randy, this was your change, right?

Yes, I'm trying to track that down...


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [Pcihpd-discuss] [PATCH 26/34] PCI: add pci_try_set_mwi

[Randy Dunlap]

On Wed, 11 Jul 2007 17:37:31 -0700 Greg KH wrote:

> On Wed, Jul 11, 2007 at 06:01:49PM -0600, Matthew Wilcox wrote:
> > On Wed, Jul 11, 2007 at 04:31:40PM -0700, Greg Kroah-Hartman wrote:
> > > As suggested by Andrew, add pci_try_set_mwi(), which does not require
> > > return-value checking.
> > 
> > Seems like a daft suggestion.  What's wrong with just removing the
> > __must_check from pci_set_mwi()?  Did it find any bugs?

(a) Alan suggested just dropping __must_check IIRC.
And David Brownell even sent a patch to do that (which Alan acked).

(b) not that I know of.

> > > --- a/drivers/scsi/lpfc/lpfc_init.c
> > > +++ b/drivers/scsi/lpfc/lpfc_init.c
> > > @@ -1578,10 +1578,7 @@ lpfc_pci_probe_one(struct pci_dev *pdev, const struct pci_device_id *pid)
> > >  	INIT_LIST_HEAD(&phba->fc_nodes);
> > >  
> > >  	pci_set_master(pdev);
> > > -	retval = pci_set_mwi(pdev);
> > > -	if (retval)
> > > -		dev_printk(KERN_WARNING, &pdev->dev,
> > > -			   "Warning: pci_set_mwi returned %d\n", retval);
> > > +	pci_try_set_mwi(pdev);
> > 
> > Why remove the warning?  Presumably people want to know if pci_set_mwi
> > failed.
> 
> Randy, this was your change, right?

Uh, I think that my thinking was like this:

pci_try_set_mwi() and pci_set_mwi() are both "try best effort"
functions.  Neither of them guarantees that pci_set_cacheline_size()
will succeed.  And in case of serious problems, pci_set_cacheline_size()
will print a (KERN_DEBUG) message.


Anyway, I don't mind restoring the former lpfc code if that is what
should be done.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [Pcihpd-discuss] [PATCH 26/34] PCI: add pci_try_set_mwi

[James Smart]

I'm agnostic on the change... As long as we get a message somewhere
when the failure is meaningful, I'm fine with this change. I didn't
like setting mwi by the driver anyway - it should have already been
done by the platform.

-- james s


Randy Dunlap wrote:
>>>> --- a/drivers/scsi/lpfc/lpfc_init.c
>>>> +++ b/drivers/scsi/lpfc/lpfc_init.c
>>>> @@ -1578,10 +1578,7 @@ lpfc_pci_probe_one(struct pci_dev *pdev, const struct pci_device_id *pid)
>>>>  	INIT_LIST_HEAD(&phba->fc_nodes);
>>>>  
>>>>  	pci_set_master(pdev);
>>>> -	retval = pci_set_mwi(pdev);
>>>> -	if (retval)
>>>> -		dev_printk(KERN_WARNING, &pdev->dev,
>>>> -			   "Warning: pci_set_mwi returned %d\n", retval);
>>>> +	pci_try_set_mwi(pdev);
>>> Why remove the warning?  Presumably people want to know if pci_set_mwi
>>> failed.
>> Randy, this was your change, right?
> 
> Uh, I think that my thinking was like this:
> 
> pci_try_set_mwi() and pci_set_mwi() are both "try best effort"
> functions.  Neither of them guarantees that pci_set_cacheline_size()
> will succeed.  And in case of serious problems, pci_set_cacheline_size()
> will print a (KERN_DEBUG) message.
> 
> 
> Anyway, I don't mind restoring the former lpfc code if that is what
> should be done.
> 
> ---
> ~Randy
> *** Remember to use Documentation/SubmitChecklist when testing your code ***
>