Patchwork [1/2] PCI: Introduce pci_alloc_dev(struct pci_bus*) to replace alloc_pci_dev()

login
register
mail settings
Submitter Gu Zheng
Date April 19, 2013, 9:44 a.m.
Message ID <51711214.8050606@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/237895/
State Rejected
Headers show

Comments

Gu Zheng - April 19, 2013, 9:44 a.m.
From 906167d9a09babbe189f62944ecb8c0b198a0f64 Mon Sep 17 00:00:00 2001
From: Gu Zheng <guz.fnst@cn.fujitsu.com>
Date: Fri, 19 Apr 2013 18:12:32 +0900
Subject: [PATCH 1/2] PCI: Introduce  pci_alloc_dev(struct pci_bus*) to replace alloc_pci_dev()

Now here we introduce a new struct pci_dev *pci_alloc_dev(struct pci_bus *bus) to replace alloc_pci_dev().
It take a "struct pci_bus *" argument, so we can alloc a pci device on a target pci bus, and it acquire
the reference of the pci_bus.
Since the old alloc_pci_dev() is exported, so we still keep it for a while but mark it as __deprecated.  

Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
---
 drivers/pci/probe.c |   21 ++++++++++++++++++++-
 include/linux/pci.h |    4 +++-
 2 files changed, 23 insertions(+), 2 deletions(-)
Bjorn Helgaas - April 19, 2013, 5:32 p.m.
On Fri, Apr 19, 2013 at 3:44 AM, Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:
> From 906167d9a09babbe189f62944ecb8c0b198a0f64 Mon Sep 17 00:00:00 2001
> From: Gu Zheng <guz.fnst@cn.fujitsu.com>
> Date: Fri, 19 Apr 2013 18:12:32 +0900
> Subject: [PATCH 1/2] PCI: Introduce  pci_alloc_dev(struct pci_bus*) to replace alloc_pci_dev()
>
> Now here we introduce a new struct pci_dev *pci_alloc_dev(struct pci_bus *bus) to replace alloc_pci_dev().
> It take a "struct pci_bus *" argument, so we can alloc a pci device on a target pci bus, and it acquire
> the reference of the pci_bus.
> Since the old alloc_pci_dev() is exported, so we still keep it for a while but mark it as __deprecated.
>
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> ---
>  drivers/pci/probe.c |   21 ++++++++++++++++++++-
>  include/linux/pci.h |    4 +++-
>  2 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index b494066..5233fb6 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1199,7 +1199,7 @@ static void pci_release_bus_bridge_dev(struct device *dev)
>         kfree(bridge);
>  }
>
> -struct pci_dev *alloc_pci_dev(void)
> +struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
>  {
>         struct pci_dev *dev;
>
> @@ -1209,6 +1209,25 @@ struct pci_dev *alloc_pci_dev(void)
>
>         INIT_LIST_HEAD(&dev->bus_list);
>
> +       if (bus) {
> +               get_device(&bus->dev);
> +               dev->bus = bus;
> +       }
> +
> +       return dev;
> +}
> +EXPORT_SYMBOL(pci_alloc_dev);
> +
> +__deprecated struct pci_dev *alloc_pci_dev(void)

I don't think there's any point in marking the function *definition*
as deprecated; it only makes sense for the declaration in the header
file, so callers of the function will generate warnings.

> +{
> +       struct pci_dev *dev;
> +       printk(KERN_DEBUG "alloc_pci_dev is deprecated, please use pci_alloc_dev(struct pci_bus *) instead!\n");

I don't want to print a message at run-time, because users will see
the message and complain about it, but they can't do anything about
it.  And neither can we, because it will only be out-of-tree modules
that call alloc_pci_dev().  The build-time warning is all we can do.

> +       dev = kzalloc(sizeof(struct pci_dev), GFP_KERNEL);
> +       if (!dev)
> +               return NULL;
> +
> +       INIT_LIST_HEAD(&dev->bus_list);

Can't you implement this as simply:

    return pci_alloc_dev(NULL);

> +
>         return dev;
>  }
>  EXPORT_SYMBOL(alloc_pci_dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 710067f..682de2b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -348,7 +348,9 @@ static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
>         return dev;
>  }
>
> -extern struct pci_dev *alloc_pci_dev(void);
> +extern struct pci_dev *pci_alloc_dev(struct pci_bus *bus);
> +
> +extern __deprecated struct pci_dev *alloc_pci_dev(void);
>
>  #define        to_pci_dev(n) container_of(n, struct pci_dev, dev)
>  #define for_each_pci_dev(d) while ((d = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, d)) != NULL)
> --
> 1.7.1
>
--
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
Mike Qiu - April 20, 2013, 2:58 a.m.
On Fri, 2013-04-19 at 11:32 -0600, Bjorn Helgaas wrote:
> On Fri, Apr 19, 2013 at 3:44 AM, Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:
> > From 906167d9a09babbe189f62944ecb8c0b198a0f64 Mon Sep 17 00:00:00 2001
> > From: Gu Zheng <guz.fnst@cn.fujitsu.com>
> > Date: Fri, 19 Apr 2013 18:12:32 +0900
> > Subject: [PATCH 1/2] PCI: Introduce  pci_alloc_dev(struct pci_bus*) to replace alloc_pci_dev()
> >
> > Now here we introduce a new struct pci_dev *pci_alloc_dev(struct pci_bus *bus) to replace alloc_pci_dev().
> > It take a "struct pci_bus *" argument, so we can alloc a pci device on a target pci bus, and it acquire
> > the reference of the pci_bus.
> > Since the old alloc_pci_dev() is exported, so we still keep it for a while but mark it as __deprecated.
> >
> > Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> > ---
> >  drivers/pci/probe.c |   21 ++++++++++++++++++++-
> >  include/linux/pci.h |    4 +++-
> >  2 files changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index b494066..5233fb6 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1199,7 +1199,7 @@ static void pci_release_bus_bridge_dev(struct device *dev)
> >         kfree(bridge);
> >  }
> >
> > -struct pci_dev *alloc_pci_dev(void)
> > +struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
> >  {
> >         struct pci_dev *dev;
> >
> > @@ -1209,6 +1209,25 @@ struct pci_dev *alloc_pci_dev(void)
> >
> >         INIT_LIST_HEAD(&dev->bus_list);
> >
> > +       if (bus) {
> > +               get_device(&bus->dev);
> > +               dev->bus = bus;
> > +       }
> > +
> > +       return dev;
> > +}
> > +EXPORT_SYMBOL(pci_alloc_dev);
> > +
> > +__deprecated struct pci_dev *alloc_pci_dev(void)
> 
> I don't think there's any point in marking the function *definition*
> as deprecated; it only makes sense for the declaration in the header
> file, so callers of the function will generate warnings.
> 
> > +{
> > +       struct pci_dev *dev;
> > +       printk(KERN_DEBUG "alloc_pci_dev is deprecated, please use pci_alloc_dev(struct pci_bus *) instead!\n");
> 
> I don't want to print a message at run-time, because users will see
> the message and complain about it, but they can't do anything about
> it.  And neither can we, because it will only be out-of-tree modules
> that call alloc_pci_dev().  The build-time warning is all we can do.
Yes, totally agree .
> 
> > +       dev = kzalloc(sizeof(struct pci_dev), GFP_KERNEL);
> > +       if (!dev)
> > +               return NULL;
> > +
> > +       INIT_LIST_HEAD(&dev->bus_list);
> 
> Can't you implement this as simply:
> 
>     return pci_alloc_dev(NULL);
Yes, I think it's better to keep the old API, and modify it to call the
new one, this makes the modules which call the old fashion API feel
smoothly. and the new modules can use the new API.
> 
> > +
> >         return dev;
> >  }
> >  EXPORT_SYMBOL(alloc_pci_dev);
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 710067f..682de2b 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -348,7 +348,9 @@ static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
> >         return dev;
> >  }
> >
> > -extern struct pci_dev *alloc_pci_dev(void);
> > +extern struct pci_dev *pci_alloc_dev(struct pci_bus *bus);
> > +
> > +extern __deprecated struct pci_dev *alloc_pci_dev(void);
> >
> >  #define        to_pci_dev(n) container_of(n, struct pci_dev, dev)
> >  #define for_each_pci_dev(d) while ((d = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, d)) != NULL)
> > --
> > 1.7.1
> >
> --
> 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
> 


--
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
Gu Zheng - April 22, 2013, 2:39 a.m.
Hi Bjorn,
   Sorry for the later reply!

On 04/20/2013 01:32 AM, Bjorn Helgaas wrote:

> On Fri, Apr 19, 2013 at 3:44 AM, Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:
>> From 906167d9a09babbe189f62944ecb8c0b198a0f64 Mon Sep 17 00:00:00 2001
>> From: Gu Zheng <guz.fnst@cn.fujitsu.com>
>> Date: Fri, 19 Apr 2013 18:12:32 +0900
>> Subject: [PATCH 1/2] PCI: Introduce  pci_alloc_dev(struct pci_bus*) to replace alloc_pci_dev()
>>
>> Now here we introduce a new struct pci_dev *pci_alloc_dev(struct pci_bus *bus) to replace alloc_pci_dev().
>> It take a "struct pci_bus *" argument, so we can alloc a pci device on a target pci bus, and it acquire
>> the reference of the pci_bus.
>> Since the old alloc_pci_dev() is exported, so we still keep it for a while but mark it as __deprecated.
>>
>> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>> ---
>>  drivers/pci/probe.c |   21 ++++++++++++++++++++-
>>  include/linux/pci.h |    4 +++-
>>  2 files changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index b494066..5233fb6 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -1199,7 +1199,7 @@ static void pci_release_bus_bridge_dev(struct device *dev)
>>         kfree(bridge);
>>  }
>>
>> -struct pci_dev *alloc_pci_dev(void)
>> +struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
>>  {
>>         struct pci_dev *dev;
>>
>> @@ -1209,6 +1209,25 @@ struct pci_dev *alloc_pci_dev(void)
>>
>>         INIT_LIST_HEAD(&dev->bus_list);
>>
>> +       if (bus) {
>> +               get_device(&bus->dev);
>> +               dev->bus = bus;
>> +       }
>> +
>> +       return dev;
>> +}
>> +EXPORT_SYMBOL(pci_alloc_dev);
>> +
>> +__deprecated struct pci_dev *alloc_pci_dev(void)
> 
> I don't think there's any point in marking the function *definition*
> as deprecated; it only makes sense for the declaration in the header
> file, so callers of the function will generate warnings.

Thanks for your explaining, I'll fix it in the right way.

> 
>> +{
>> +       struct pci_dev *dev;
>> +       printk(KERN_DEBUG "alloc_pci_dev is deprecated, please use pci_alloc_dev(struct pci_bus *) instead!\n");
> 
> I don't want to print a message at run-time, because users will see
> the message and complain about it, but they can't do anything about
> it.  And neither can we, because it will only be out-of-tree modules
> that call alloc_pci_dev().  The build-time warning is all we can do.

Yeah, it seems useless here. I'll remove it. 
 

> 
>> +       dev = kzalloc(sizeof(struct pci_dev), GFP_KERNEL);
>> +       if (!dev)
>> +               return NULL;
>> +
>> +       INIT_LIST_HEAD(&dev->bus_list);
> 
> Can't you implement this as simply:
> 
>     return pci_alloc_dev(NULL);

Agree, it can simplify the code, and make the later removal of the old alloc_pci_dev() smoothly.

> 
>> +
>>         return dev;
>>  }
>>  EXPORT_SYMBOL(alloc_pci_dev);
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 710067f..682de2b 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -348,7 +348,9 @@ static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
>>         return dev;
>>  }
>>
>> -extern struct pci_dev *alloc_pci_dev(void);
>> +extern struct pci_dev *pci_alloc_dev(struct pci_bus *bus);
>> +
>> +extern __deprecated struct pci_dev *alloc_pci_dev(void);
>>
>>  #define        to_pci_dev(n) container_of(n, struct pci_dev, dev)
>>  #define for_each_pci_dev(d) while ((d = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, d)) != NULL)
>> --
>> 1.7.1
>>
> 


--
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
Gu Zheng - April 22, 2013, 2:40 a.m.
On 04/20/2013 10:58 AM, Mike Qiu wrote:

> On Fri, 2013-04-19 at 11:32 -0600, Bjorn Helgaas wrote:
>> On Fri, Apr 19, 2013 at 3:44 AM, Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:
>>> From 906167d9a09babbe189f62944ecb8c0b198a0f64 Mon Sep 17 00:00:00 2001
>>> From: Gu Zheng <guz.fnst@cn.fujitsu.com>
>>> Date: Fri, 19 Apr 2013 18:12:32 +0900
>>> Subject: [PATCH 1/2] PCI: Introduce  pci_alloc_dev(struct pci_bus*) to replace alloc_pci_dev()
>>>
>>> Now here we introduce a new struct pci_dev *pci_alloc_dev(struct pci_bus *bus) to replace alloc_pci_dev().
>>> It take a "struct pci_bus *" argument, so we can alloc a pci device on a target pci bus, and it acquire
>>> the reference of the pci_bus.
>>> Since the old alloc_pci_dev() is exported, so we still keep it for a while but mark it as __deprecated.
>>>
>>> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>>> ---
>>>  drivers/pci/probe.c |   21 ++++++++++++++++++++-
>>>  include/linux/pci.h |    4 +++-
>>>  2 files changed, 23 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>> index b494066..5233fb6 100644
>>> --- a/drivers/pci/probe.c
>>> +++ b/drivers/pci/probe.c
>>> @@ -1199,7 +1199,7 @@ static void pci_release_bus_bridge_dev(struct device *dev)
>>>         kfree(bridge);
>>>  }
>>>
>>> -struct pci_dev *alloc_pci_dev(void)
>>> +struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
>>>  {
>>>         struct pci_dev *dev;
>>>
>>> @@ -1209,6 +1209,25 @@ struct pci_dev *alloc_pci_dev(void)
>>>
>>>         INIT_LIST_HEAD(&dev->bus_list);
>>>
>>> +       if (bus) {
>>> +               get_device(&bus->dev);
>>> +               dev->bus = bus;
>>> +       }
>>> +
>>> +       return dev;
>>> +}
>>> +EXPORT_SYMBOL(pci_alloc_dev);
>>> +
>>> +__deprecated struct pci_dev *alloc_pci_dev(void)
>>
>> I don't think there's any point in marking the function *definition*
>> as deprecated; it only makes sense for the declaration in the header
>> file, so callers of the function will generate warnings.
>>
>>> +{
>>> +       struct pci_dev *dev;
>>> +       printk(KERN_DEBUG "alloc_pci_dev is deprecated, please use pci_alloc_dev(struct pci_bus *) instead!\n");
>>
>> I don't want to print a message at run-time, because users will see
>> the message and complain about it, but they can't do anything about
>> it.  And neither can we, because it will only be out-of-tree modules
>> that call alloc_pci_dev().  The build-time warning is all we can do.
> Yes, totally agree .
>>
>>> +       dev = kzalloc(sizeof(struct pci_dev), GFP_KERNEL);
>>> +       if (!dev)
>>> +               return NULL;
>>> +
>>> +       INIT_LIST_HEAD(&dev->bus_list);
>>
>> Can't you implement this as simply:
>>
>>     return pci_alloc_dev(NULL);
> Yes, I think it's better to keep the old API, and modify it to call the
> new one, this makes the modules which call the old fashion API feel
> smoothly. and the new modules can use the new API.

Hi Mike,
    You are right! Thanks for your suggestion!

Regards,
Gu

>>
>>> +
>>>         return dev;
>>>  }
>>>  EXPORT_SYMBOL(alloc_pci_dev);
>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>> index 710067f..682de2b 100644
>>> --- a/include/linux/pci.h
>>> +++ b/include/linux/pci.h
>>> @@ -348,7 +348,9 @@ static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
>>>         return dev;
>>>  }
>>>
>>> -extern struct pci_dev *alloc_pci_dev(void);
>>> +extern struct pci_dev *pci_alloc_dev(struct pci_bus *bus);
>>> +
>>> +extern __deprecated struct pci_dev *alloc_pci_dev(void);
>>>
>>>  #define        to_pci_dev(n) container_of(n, struct pci_dev, dev)
>>>  #define for_each_pci_dev(d) while ((d = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, d)) != NULL)
>>> --
>>> 1.7.1
>>>
>> --
>> 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
>>
> 
> 
> 


--
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

Patch

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b494066..5233fb6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1199,7 +1199,7 @@  static void pci_release_bus_bridge_dev(struct device *dev)
 	kfree(bridge);
 }
 
-struct pci_dev *alloc_pci_dev(void)
+struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
 {
 	struct pci_dev *dev;
 
@@ -1209,6 +1209,25 @@  struct pci_dev *alloc_pci_dev(void)
 
 	INIT_LIST_HEAD(&dev->bus_list);
 
+	if (bus) {
+		get_device(&bus->dev);
+		dev->bus = bus;
+	}
+
+	return dev;
+}
+EXPORT_SYMBOL(pci_alloc_dev);
+
+__deprecated struct pci_dev *alloc_pci_dev(void)
+{
+	struct pci_dev *dev;
+	printk(KERN_DEBUG "alloc_pci_dev is deprecated, please use pci_alloc_dev(struct pci_bus *) instead!\n");
+	dev = kzalloc(sizeof(struct pci_dev), GFP_KERNEL);
+	if (!dev)
+		return NULL;
+
+	INIT_LIST_HEAD(&dev->bus_list);
+
 	return dev;
 }
 EXPORT_SYMBOL(alloc_pci_dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 710067f..682de2b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -348,7 +348,9 @@  static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
 	return dev;
 }
 
-extern struct pci_dev *alloc_pci_dev(void);
+extern struct pci_dev *pci_alloc_dev(struct pci_bus *bus);
+
+extern __deprecated struct pci_dev *alloc_pci_dev(void);
 
 #define	to_pci_dev(n) container_of(n, struct pci_dev, dev)
 #define for_each_pci_dev(d) while ((d = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, d)) != NULL)