[PATCH] staging: comedi: resolve section mismatch in s626

[PATCH] staging: comedi: resolve section mismatch in s626

[Gerard Snitselaar]

s626_attach is referencing s626_pci_table which is annotated
__devinitconst. Put pci vendor/device ids in the s626_board struct and
use s626_boards instead similar to what is done in gsc_hpdi.

Signed-off-by: Gerard Snitselaar <dev@snitselaar.org>
---
 drivers/staging/comedi/drivers/s626.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
index 23fc64b..6c017a3 100644
--- a/drivers/staging/comedi/drivers/s626.c
+++ b/drivers/staging/comedi/drivers/s626.c
@@ -85,6 +85,10 @@ MODULE_LICENSE("GPL");
 
 struct s626_board {
 	const char *name;
+	int vendor_id;
+	int device_id;
+	int subvendor_id;
+	int subdevice_id;
 	int ai_chans;
 	int ai_bits;
 	int ao_chans;
@@ -97,6 +101,10 @@ struct s626_board {
 static const struct s626_board s626_boards[] = {
 	{
 	 .name = "s626",
+	 .vendor_id = PCI_VENDOR_ID_S626,
+	 .device_id = PCI_DEVICE_ID_S626,
+	 .subvendor_id = PCI_SUBVENDOR_ID_S626,
+	 .subdevice_id = PCI_SUBDEVICE_ID_S626,
 	 .ai_chans = S626_ADC_CHANNELS,
 	 .ai_bits = 14,
 	 .ao_chans = S626_DAC_CHANNELS,
@@ -110,6 +118,8 @@ static const struct s626_board s626_boards[] = {
 #define thisboard ((const struct s626_board *)dev->board_ptr)
 #define PCI_VENDOR_ID_S626 0x1131
 #define PCI_DEVICE_ID_S626 0x7146
+#define PCI_SUBVENDOR_ID_S626 0x6000
+#define PCI_SUBDEVICE_ID_S626 0x0272
 
 /*
  * For devices with vendor:device id == 0x1131:0x7146 you must specify
@@ -117,7 +127,7 @@ static const struct s626_board s626_boards[] = {
  * Philips SAA7146 media/dvb based cards.
  */
 static DEFINE_PCI_DEVICE_TABLE(s626_pci_table) = {
-	{PCI_VENDOR_ID_S626, PCI_DEVICE_ID_S626, 0x6000, 0x0272, 0, 0, 0},
+	{PCI_VENDOR_ID_S626, PCI_DEVICE_ID_S626, PCI_SUBVENDOR_ID_S626, PCI_SUBDEVICE_ID_S626, 0, 0, 0},
 	{0}
 };
 
@@ -554,17 +564,17 @@ static int s626_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 	resource_size_t resourceStart;
 	dma_addr_t appdma;
 	struct comedi_subdevice *s;
-	const struct pci_device_id *ids;
 	struct pci_dev *pdev = NULL;
 
 	if (alloc_private(dev, sizeof(struct s626_private)) < 0)
 		return -ENOMEM;
 
-	for (i = 0; i < (ARRAY_SIZE(s626_pci_table) - 1) && !pdev; i++) {
-		ids = &s626_pci_table[i];
+	for (i = 0; i < ARRAY_SIZE(s626_boards) && !pdev; i++) {
 		do {
-			pdev = pci_get_subsys(ids->vendor, ids->device,
-					      ids->subvendor, ids->subdevice,
+			pdev = pci_get_subsys(s626_boards[i].vendor_id,
+					      s626_boards[i].device_id,
+					      s626_boards[i].subvendor_id,
+					      s626_boards[i].subdevice_id,
 					      pdev);
 
 			if ((it->options[0] || it->options[1]) && pdev) {
-- 
1.7.7.6

[PATCH v2] staging: comedi: resolve section mismatch in s626

[Gerard Snitselaar]

s626_attach is referencing s626_pci_table which is annotated
__devinitconst. Put pci vendor/device ids in the s626_board struct and
use s626_boards instead similar to what is done in gsc_hpdi.

v2: I had moved the PCI id defines to s626.h earlier, but later
decided to leave them in s626.c and forgot to move them up above where
they are being used in s626_boards.

Signed-off-by: Gerard Snitselaar <dev@snitselaar.org>
---
 drivers/staging/comedi/drivers/s626.c |   27 +++++++++++++++++++--------
 1 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
index 23fc64b..e531f1c 100644
--- a/drivers/staging/comedi/drivers/s626.c
+++ b/drivers/staging/comedi/drivers/s626.c
@@ -83,8 +83,17 @@ MODULE_AUTHOR("Gianluca Palli <gpalli@deis.unibo.it>");
 MODULE_DESCRIPTION("Sensoray 626 Comedi driver module");
 MODULE_LICENSE("GPL");
 
+#define PCI_VENDOR_ID_S626 0x1131
+#define PCI_DEVICE_ID_S626 0x7146
+#define PCI_SUBVENDOR_ID_S626 0x6000
+#define PCI_SUBDEVICE_ID_S626 0x0272
+
 struct s626_board {
        const char *name;
+       int vendor_id;
+       int device_id;
+       int subvendor_id;
+       int subdevice_id;
        int ai_chans;
        int ai_bits;
        int ao_chans;
@@ -97,6 +106,10 @@ struct s626_board {
 static const struct s626_board s626_boards[] = {
        {
         .name = "s626",
+        .vendor_id = PCI_VENDOR_ID_S626,
+        .device_id = PCI_DEVICE_ID_S626,
+        .subvendor_id = PCI_SUBVENDOR_ID_S626,
+        .subdevice_id = PCI_SUBDEVICE_ID_S626,
         .ai_chans = S626_ADC_CHANNELS,
         .ai_bits = 14,
         .ao_chans = S626_DAC_CHANNELS,
@@ -108,8 +121,6 @@ static const struct s626_board s626_boards[] = {
 };
 
 #define thisboard ((const struct s626_board *)dev->board_ptr)
-#define PCI_VENDOR_ID_S626 0x1131
-#define PCI_DEVICE_ID_S626 0x7146
 
 /*
  * For devices with vendor:device id == 0x1131:0x7146 you must specify
@@ -117,7 +128,7 @@ static const struct s626_board s626_boards[] = {
  * Philips SAA7146 media/dvb based cards.
  */
 static DEFINE_PCI_DEVICE_TABLE(s626_pci_table) = {
-       {PCI_VENDOR_ID_S626, PCI_DEVICE_ID_S626, 0x6000, 0x0272, 0, 0, 0},
+       {PCI_VENDOR_ID_S626, PCI_DEVICE_ID_S626, PCI_SUBVENDOR_ID_S626, PCI_SUBDEVICE_ID_S626, 0, 0, 0},
        {0}
 };
 
@@ -554,17 +565,17 @@ static int s626_attach(struct comedi_device *dev, struct comedi_devconfig *it)
        resource_size_t resourceStart;
        dma_addr_t appdma;
        struct comedi_subdevice *s;
-       const struct pci_device_id *ids;
        struct pci_dev *pdev = NULL;
 
        if (alloc_private(dev, sizeof(struct s626_private)) < 0)
                return -ENOMEM;
 
-       for (i = 0; i < (ARRAY_SIZE(s626_pci_table) - 1) && !pdev; i++) {
-               ids = &s626_pci_table[i];
+       for (i = 0; i < ARRAY_SIZE(s626_boards) && !pdev; i++) {
                do {
-                       pdev = pci_get_subsys(ids->vendor, ids->device,
-                                             ids->subvendor, ids->subdevice,
+                       pdev = pci_get_subsys(s626_boards[i].vendor_id,
+                                             s626_boards[i].device_id,
+                                             s626_boards[i].subvendor_id,
+                                             s626_boards[i].subdevice_id,
                                              pdev);
 
                        if ((it->options[0] || it->options[1]) && pdev) {
-- 
1.7.7.6

Re: [PATCH v2] staging: comedi: resolve section mismatch in s626

[Greg KH]

On Sun, Mar 18, 2012 at 08:09:25PM -0700, Gerard Snitselaar wrote:
> s626_attach is referencing s626_pci_table which is annotated
> __devinitconst. Put pci vendor/device ids in the s626_board struct and
> use s626_boards instead similar to what is done in gsc_hpdi.
> 
> v2: I had moved the PCI id defines to s626.h earlier, but later
> decided to leave them in s626.c and forgot to move them up above where
> they are being used in s626_boards.
> 
> Signed-off-by: Gerard Snitselaar <dev@snitselaar.org>
> ---
>  drivers/staging/comedi/drivers/s626.c |   27 +++++++++++++++++++--------
>  1 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
> index 23fc64b..e531f1c 100644
> --- a/drivers/staging/comedi/drivers/s626.c
> +++ b/drivers/staging/comedi/drivers/s626.c
> @@ -83,8 +83,17 @@ MODULE_AUTHOR("Gianluca Palli <gpalli@deis.unibo.it>");
>  MODULE_DESCRIPTION("Sensoray 626 Comedi driver module");
>  MODULE_LICENSE("GPL");
>  
> +#define PCI_VENDOR_ID_S626 0x1131
> +#define PCI_DEVICE_ID_S626 0x7146
> +#define PCI_SUBVENDOR_ID_S626 0x6000
> +#define PCI_SUBDEVICE_ID_S626 0x0272
> +
>  struct s626_board {
>         const char *name;
> +       int vendor_id;
> +       int device_id;
> +       int subvendor_id;
> +       int subdevice_id;
>         int ai_chans;
>         int ai_bits;
>         int ao_chans;
> @@ -97,6 +106,10 @@ struct s626_board {
>  static const struct s626_board s626_boards[] = {
>         {
>          .name = "s626",
> +        .vendor_id = PCI_VENDOR_ID_S626,
> +        .device_id = PCI_DEVICE_ID_S626,
> +        .subvendor_id = PCI_SUBVENDOR_ID_S626,
> +        .subdevice_id = PCI_SUBDEVICE_ID_S626,
>          .ai_chans = S626_ADC_CHANNELS,
>          .ai_bits = 14,
>          .ao_chans = S626_DAC_CHANNELS,
> @@ -108,8 +121,6 @@ static const struct s626_board s626_boards[] = {
>  };
>  
>  #define thisboard ((const struct s626_board *)dev->board_ptr)
> -#define PCI_VENDOR_ID_S626 0x1131
> -#define PCI_DEVICE_ID_S626 0x7146
>  
>  /*
>   * For devices with vendor:device id == 0x1131:0x7146 you must specify
> @@ -117,7 +128,7 @@ static const struct s626_board s626_boards[] = {
>   * Philips SAA7146 media/dvb based cards.
>   */
>  static DEFINE_PCI_DEVICE_TABLE(s626_pci_table) = {
> -       {PCI_VENDOR_ID_S626, PCI_DEVICE_ID_S626, 0x6000, 0x0272, 0, 0, 0},
> +       {PCI_VENDOR_ID_S626, PCI_DEVICE_ID_S626, PCI_SUBVENDOR_ID_S626, PCI_SUBDEVICE_ID_S626, 0, 0, 0},
>         {0}
>  };
>  
> @@ -554,17 +565,17 @@ static int s626_attach(struct comedi_device *dev, struct comedi_devconfig *it)
>         resource_size_t resourceStart;
>         dma_addr_t appdma;
>         struct comedi_subdevice *s;
> -       const struct pci_device_id *ids;
>         struct pci_dev *pdev = NULL;
>  
>         if (alloc_private(dev, sizeof(struct s626_private)) < 0)
>                 return -ENOMEM;
>  
> -       for (i = 0; i < (ARRAY_SIZE(s626_pci_table) - 1) && !pdev; i++) {
> -               ids = &s626_pci_table[i];
> +       for (i = 0; i < ARRAY_SIZE(s626_boards) && !pdev; i++) {
>                 do {
> -                       pdev = pci_get_subsys(ids->vendor, ids->device,
> -                                             ids->subvendor, ids->subdevice,
> +                       pdev = pci_get_subsys(s626_boards[i].vendor_id,
> +                                             s626_boards[i].device_id,
> +                                             s626_boards[i].subvendor_id,
> +                                             s626_boards[i].subdevice_id,
>                                               pdev);

Ick, why is this loop even needed?  We are only here if the pci device
is present in the system so this shouldn't be needed at all, right?

Or is this a bit more complex than I am making it out to be?

greg k-h

Re: [PATCH v2] staging: comedi: resolve section mismatch in s626

[Gerard Snitselaar]

On Mon, Mar 19, 2012 at 09:31:03AM -0700, Greg KH wrote:
> 
> Ick, why is this loop even needed?  We are only here if the pci device
> is present in the system so this shouldn't be needed at all, right?
> 
> Or is this a bit more complex than I am making it out to be?
> 
> greg k-h

Most likely not. I will take a look at some of the other drivers in
comedi and see how the attach code looks there. I believe the code
section in hpdi_attach() was written by the same person. Unfortunately
I don't have a device to actually play around and see what changes are
doing.

Re: [PATCH v2] staging: comedi: resolve section mismatch in s626

[Gerard Snitselaar]

On Mon, Mar 19, 2012 at 09:43:25AM -0700, Gerard Snitselaar wrote:
> On Mon, Mar 19, 2012 at 09:31:03AM -0700, Greg KH wrote:
> > 
> > Ick, why is this loop even needed?  We are only here if the pci device
> > is present in the system so this shouldn't be needed at all, right?
> > 
> > Or is this a bit more complex than I am making it out to be?
> > 
> > greg k-h
> 
> Most likely not. I will take a look at some of the other drivers in
> comedi and see how the attach code looks there. I believe the code
> section in hpdi_attach() was written by the same person. Unfortunately
> I don't have a device to actually play around and see what changes are
> doing.
> 

I looked at this a bit more. It looks like they lose visibility to the
pci_dev structure.

*_probe() 
   comedi_pci_auto_config()     pci_dev
      comedi_auto_config()      pci_dev->dev
         comedi_device_attach() ??
            driv->attach()      ?? <= iterate through pci devices.

Most of the examples I have looked at so far use for_each_pci_dev() to
find the device, and s626 shortcuts it a bit by directly making calls
to pci_get_subsys() with specific ids. They all verify they have the
right device by checking the bus and slot that are grabbed from the
pci_dev in comedi_pci_auto_config() and passed down.

Re: [PATCH v2] staging: comedi: resolve section mismatch in s626

[Greg KH]

On Mon, Mar 19, 2012 at 03:46:49PM -0700, Gerard Snitselaar wrote:
> On Mon, Mar 19, 2012 at 09:43:25AM -0700, Gerard Snitselaar wrote:
> > On Mon, Mar 19, 2012 at 09:31:03AM -0700, Greg KH wrote:
> > > 
> > > Ick, why is this loop even needed?  We are only here if the pci device
> > > is present in the system so this shouldn't be needed at all, right?
> > > 
> > > Or is this a bit more complex than I am making it out to be?
> > > 
> > > greg k-h
> > 
> > Most likely not. I will take a look at some of the other drivers in
> > comedi and see how the attach code looks there. I believe the code
> > section in hpdi_attach() was written by the same person. Unfortunately
> > I don't have a device to actually play around and see what changes are
> > doing.
> > 
> 
> I looked at this a bit more. It looks like they lose visibility to the
> pci_dev structure.
> 
> *_probe() 
>    comedi_pci_auto_config()     pci_dev
>       comedi_auto_config()      pci_dev->dev
>          comedi_device_attach() ??
>             driv->attach()      ?? <= iterate through pci devices.
> 
> Most of the examples I have looked at so far use for_each_pci_dev() to
> find the device, and s626 shortcuts it a bit by directly making calls
> to pci_get_subsys() with specific ids. They all verify they have the
> right device by checking the bus and slot that are grabbed from the
> pci_dev in comedi_pci_auto_config() and passed down.

Ugh, surely there's a way to keep the pci dev through the
comedi_device_attach() call, right?

How about asking this on the comedi mailing list?

Re: [PATCH v2] staging: comedi: resolve section mismatch in s626

[Ian Abbott]

On 2012-03-19 23:26, Greg KH wrote:
> On Mon, Mar 19, 2012 at 03:46:49PM -0700, Gerard Snitselaar wrote:
>> On Mon, Mar 19, 2012 at 09:43:25AM -0700, Gerard Snitselaar wrote:
>>> On Mon, Mar 19, 2012 at 09:31:03AM -0700, Greg KH wrote:
>>>>
>>>> Ick, why is this loop even needed?  We are only here if the pci device
>>>> is present in the system so this shouldn't be needed at all, right?
>>>>
>>>> Or is this a bit more complex than I am making it out to be?
>>>>
>>>> greg k-h
>>>
>>> Most likely not. I will take a look at some of the other drivers in
>>> comedi and see how the attach code looks there. I believe the code
>>> section in hpdi_attach() was written by the same person. Unfortunately
>>> I don't have a device to actually play around and see what changes are
>>> doing.
>>>
>>
>> I looked at this a bit more. It looks like they lose visibility to the
>> pci_dev structure.
>>
>> *_probe()
>>     comedi_pci_auto_config()     pci_dev
>>        comedi_auto_config()      pci_dev->dev
>>           comedi_device_attach() ??
>>              driv->attach()      ??<= iterate through pci devices.
>>
>> Most of the examples I have looked at so far use for_each_pci_dev() to
>> find the device, and s626 shortcuts it a bit by directly making calls
>> to pci_get_subsys() with specific ids. They all verify they have the
>> right device by checking the bus and slot that are grabbed from the
>> pci_dev in comedi_pci_auto_config() and passed down.
>
> Ugh, surely there's a way to keep the pci dev through the
> comedi_device_attach() call, right?

comedi_device_attach() is also called for the COMEDI_DEVCONFIG ioctl for 
"manually" configuring a comedi device, and that has no idea about 
struct pci_dev, etc.

I recently posted a series of patches that allows lower-level comedi 
drivers to supply separate hooks for auto-configuring PCI devices or USB 
devices without abusing the old "manual configuration" code paths, see 
<http://driverdev.linuxdriverproject.org/pipermail/devel/2012-March/025331.html>.

The old loop that searches the PCI bus is still needed for the "manual 
configuration" code path.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH v2] staging: comedi: resolve section mismatch in s626

[Greg KH]

On Mon, Apr 02, 2012 at 11:48:54AM +0100, Ian Abbott wrote:
> On 2012-03-19 23:26, Greg KH wrote:
> >On Mon, Mar 19, 2012 at 03:46:49PM -0700, Gerard Snitselaar wrote:
> >>On Mon, Mar 19, 2012 at 09:43:25AM -0700, Gerard Snitselaar wrote:
> >>>On Mon, Mar 19, 2012 at 09:31:03AM -0700, Greg KH wrote:
> >>>>
> >>>>Ick, why is this loop even needed?  We are only here if the pci device
> >>>>is present in the system so this shouldn't be needed at all, right?
> >>>>
> >>>>Or is this a bit more complex than I am making it out to be?
> >>>>
> >>>>greg k-h
> >>>
> >>>Most likely not. I will take a look at some of the other drivers in
> >>>comedi and see how the attach code looks there. I believe the code
> >>>section in hpdi_attach() was written by the same person. Unfortunately
> >>>I don't have a device to actually play around and see what changes are
> >>>doing.
> >>>
> >>
> >>I looked at this a bit more. It looks like they lose visibility to the
> >>pci_dev structure.
> >>
> >>*_probe()
> >>    comedi_pci_auto_config()     pci_dev
> >>       comedi_auto_config()      pci_dev->dev
> >>          comedi_device_attach() ??
> >>             driv->attach()      ??<= iterate through pci devices.
> >>
> >>Most of the examples I have looked at so far use for_each_pci_dev() to
> >>find the device, and s626 shortcuts it a bit by directly making calls
> >>to pci_get_subsys() with specific ids. They all verify they have the
> >>right device by checking the bus and slot that are grabbed from the
> >>pci_dev in comedi_pci_auto_config() and passed down.
> >
> >Ugh, surely there's a way to keep the pci dev through the
> >comedi_device_attach() call, right?
> 
> comedi_device_attach() is also called for the COMEDI_DEVCONFIG ioctl
> for "manually" configuring a comedi device, and that has no idea
> about struct pci_dev, etc.
> 
> I recently posted a series of patches that allows lower-level comedi
> drivers to supply separate hooks for auto-configuring PCI devices or
> USB devices without abusing the old "manual configuration" code
> paths, see <http://driverdev.linuxdriverproject.org/pipermail/devel/2012-March/025331.html>.
> 
> The old loop that searches the PCI bus is still needed for the
> "manual configuration" code path.

So, now that I've applied your patches, this patch isn't needed anymore,
right?  Or should it be reworked to use the new interfaces?

thanks,

greg k-h

Re: [PATCH v2] staging: comedi: resolve section mismatch in s626

[Gerard Snitselaar]

On Tue Apr 10 12, Greg KH wrote:
> On Mon, Apr 02, 2012 at 11:48:54AM +0100, Ian Abbott wrote:
> > On 2012-03-19 23:26, Greg KH wrote:
> > >On Mon, Mar 19, 2012 at 03:46:49PM -0700, Gerard Snitselaar wrote:
> > >>On Mon, Mar 19, 2012 at 09:43:25AM -0700, Gerard Snitselaar wrote:
> > >>>On Mon, Mar 19, 2012 at 09:31:03AM -0700, Greg KH wrote:
> > >>>>
> > >>>>Ick, why is this loop even needed?  We are only here if the pci device
> > >>>>is present in the system so this shouldn't be needed at all, right?
> > >>>>
> > >>>>Or is this a bit more complex than I am making it out to be?
> > >>>>
> > >>>>greg k-h
> > >>>
> > >>>Most likely not. I will take a look at some of the other drivers in
> > >>>comedi and see how the attach code looks there. I believe the code
> > >>>section in hpdi_attach() was written by the same person. Unfortunately
> > >>>I don't have a device to actually play around and see what changes are
> > >>>doing.
> > >>>
> > >>
> > >>I looked at this a bit more. It looks like they lose visibility to the
> > >>pci_dev structure.
> > >>
> > >>*_probe()
> > >>    comedi_pci_auto_config()     pci_dev
> > >>       comedi_auto_config()      pci_dev->dev
> > >>          comedi_device_attach() ??
> > >>             driv->attach()      ??<= iterate through pci devices.
> > >>
> > >>Most of the examples I have looked at so far use for_each_pci_dev() to
> > >>find the device, and s626 shortcuts it a bit by directly making calls
> > >>to pci_get_subsys() with specific ids. They all verify they have the
> > >>right device by checking the bus and slot that are grabbed from the
> > >>pci_dev in comedi_pci_auto_config() and passed down.
> > >
> > >Ugh, surely there's a way to keep the pci dev through the
> > >comedi_device_attach() call, right?
> > 
> > comedi_device_attach() is also called for the COMEDI_DEVCONFIG ioctl
> > for "manually" configuring a comedi device, and that has no idea
> > about struct pci_dev, etc.
> > 
> > I recently posted a series of patches that allows lower-level comedi
> > drivers to supply separate hooks for auto-configuring PCI devices or
> > USB devices without abusing the old "manual configuration" code
> > paths, see <http://driverdev.linuxdriverproject.org/pipermail/devel/2012-March/025331.html>.
> > 
> > The old loop that searches the PCI bus is still needed for the
> > "manual configuration" code path.
> 
> So, now that I've applied your patches, this patch isn't needed anymore,
> right?  Or should it be reworked to use the new interfaces?
> 
> thanks,
> 
> greg k-h
> 

I'd say drop my patch and if there is still a mismatch problem it can
probably be resolved in a better way.

gerard

Re: [PATCH v2] staging: comedi: resolve section mismatch in s626

[Ian Abbott]

On 2012/04/10 07:24 PM, Greg KH wrote:
> On Mon, Apr 02, 2012 at 11:48:54AM +0100, Ian Abbott wrote:
>> On 2012-03-19 23:26, Greg KH wrote:
>>> On Mon, Mar 19, 2012 at 03:46:49PM -0700, Gerard Snitselaar wrote:
>>>> I looked at this a bit more. It looks like they lose visibility to the
>>>> pci_dev structure.
>>>>
>>>> *_probe()
>>>>    comedi_pci_auto_config()     pci_dev
>>>>       comedi_auto_config()      pci_dev->dev
>>>>          comedi_device_attach() ??
>>>>             driv->attach()      ??<= iterate through pci devices.
>>>>
>>>> Most of the examples I have looked at so far use for_each_pci_dev() to
>>>> find the device, and s626 shortcuts it a bit by directly making calls
>>>> to pci_get_subsys() with specific ids. They all verify they have the
>>>> right device by checking the bus and slot that are grabbed from the
>>>> pci_dev in comedi_pci_auto_config() and passed down.
>>>
>>> Ugh, surely there's a way to keep the pci dev through the
>>> comedi_device_attach() call, right?
>>
>> comedi_device_attach() is also called for the COMEDI_DEVCONFIG ioctl
>> for "manually" configuring a comedi device, and that has no idea
>> about struct pci_dev, etc.
>>
>> I recently posted a series of patches that allows lower-level comedi
>> drivers to supply separate hooks for auto-configuring PCI devices or
>> USB devices without abusing the old "manual configuration" code
>> paths, see <http://driverdev.linuxdriverproject.org/pipermail/devel/2012-March/025331.html>.
>>
>> The old loop that searches the PCI bus is still needed for the
>> "manual configuration" code path.
>
> So, now that I've applied your patches, this patch isn't needed anymore,
> right?  Or should it be reworked to use the new interfaces?

I think it's still needed as that the comedi_driver->attach() hook is 
still needed to support manual configuration by the COMEDI_DEVCONFIG 
ioctl, and the existing code really shouldn't be looking in 
s626_pci_table[] as it's tagged __devinitconst.

I never saw any mismatched section warnings when I compiled a linux-next 
kernel on my system with a full set of comedi drivers, even with 
CONFIG_DEBUG_SECTION_MISMATCH configured.  I'm not sure why not, unless 
it has something to do with me building the kernel with a separate build 
directory.

The new comedi_driver->attach_pci() hook should be implemented for this
driver as well, and it's on my "todo" list (in my head!).

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>             )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587              )=-

Re: [PATCH v2] staging: comedi: resolve section mismatch in s626

[Greg KH]

On Thu, Apr 12, 2012 at 02:03:16PM +0100, Ian Abbott wrote:
> On 2012/04/10 07:24 PM, Greg KH wrote:
> >On Mon, Apr 02, 2012 at 11:48:54AM +0100, Ian Abbott wrote:
> >>On 2012-03-19 23:26, Greg KH wrote:
> >>>On Mon, Mar 19, 2012 at 03:46:49PM -0700, Gerard Snitselaar wrote:
> >>>>I looked at this a bit more. It looks like they lose visibility to the
> >>>>pci_dev structure.
> >>>>
> >>>>*_probe()
> >>>>   comedi_pci_auto_config()     pci_dev
> >>>>      comedi_auto_config()      pci_dev->dev
> >>>>         comedi_device_attach() ??
> >>>>            driv->attach()      ??<= iterate through pci devices.
> >>>>
> >>>>Most of the examples I have looked at so far use for_each_pci_dev() to
> >>>>find the device, and s626 shortcuts it a bit by directly making calls
> >>>>to pci_get_subsys() with specific ids. They all verify they have the
> >>>>right device by checking the bus and slot that are grabbed from the
> >>>>pci_dev in comedi_pci_auto_config() and passed down.
> >>>
> >>>Ugh, surely there's a way to keep the pci dev through the
> >>>comedi_device_attach() call, right?
> >>
> >>comedi_device_attach() is also called for the COMEDI_DEVCONFIG ioctl
> >>for "manually" configuring a comedi device, and that has no idea
> >>about struct pci_dev, etc.
> >>
> >>I recently posted a series of patches that allows lower-level comedi
> >>drivers to supply separate hooks for auto-configuring PCI devices or
> >>USB devices without abusing the old "manual configuration" code
> >>paths, see <http://driverdev.linuxdriverproject.org/pipermail/devel/2012-March/025331.html>.
> >>
> >>The old loop that searches the PCI bus is still needed for the
> >>"manual configuration" code path.
> >
> >So, now that I've applied your patches, this patch isn't needed anymore,
> >right?  Or should it be reworked to use the new interfaces?
> 
> I think it's still needed as that the comedi_driver->attach() hook
> is still needed to support manual configuration by the
> COMEDI_DEVCONFIG ioctl, and the existing code really shouldn't be
> looking in s626_pci_table[] as it's tagged __devinitconst.
> 
> I never saw any mismatched section warnings when I compiled a
> linux-next kernel on my system with a full set of comedi drivers,
> even with CONFIG_DEBUG_SECTION_MISMATCH configured.  I'm not sure
> why not, unless it has something to do with me building the kernel
> with a separate build directory.

Yes, I don't see the mismatch either, so I'm not going to apply this
patch, sorry, as I don't see what it is trying to fix at all.

greg k-h

Re: [PATCH v2] staging: comedi: resolve section mismatch in s626

[Gerard Snitselaar]

On Wed Apr 18 12, Greg KH wrote:
> On Thu, Apr 12, 2012 at 02:03:16PM +0100, Ian Abbott wrote:
> > On 2012/04/10 07:24 PM, Greg KH wrote:
> > >On Mon, Apr 02, 2012 at 11:48:54AM +0100, Ian Abbott wrote:
> > >>On 2012-03-19 23:26, Greg KH wrote:
> > >>>On Mon, Mar 19, 2012 at 03:46:49PM -0700, Gerard Snitselaar wrote:
> > >>>>I looked at this a bit more. It looks like they lose visibility to the
> > >>>>pci_dev structure.
> > >>>>
> > >>>>*_probe()
> > >>>>   comedi_pci_auto_config()     pci_dev
> > >>>>      comedi_auto_config()      pci_dev->dev
> > >>>>         comedi_device_attach() ??
> > >>>>            driv->attach()      ??<= iterate through pci devices.
> > >>>>
> > >>>>Most of the examples I have looked at so far use for_each_pci_dev() to
> > >>>>find the device, and s626 shortcuts it a bit by directly making calls
> > >>>>to pci_get_subsys() with specific ids. They all verify they have the
> > >>>>right device by checking the bus and slot that are grabbed from the
> > >>>>pci_dev in comedi_pci_auto_config() and passed down.
> > >>>
> > >>>Ugh, surely there's a way to keep the pci dev through the
> > >>>comedi_device_attach() call, right?
> > >>
> > >>comedi_device_attach() is also called for the COMEDI_DEVCONFIG ioctl
> > >>for "manually" configuring a comedi device, and that has no idea
> > >>about struct pci_dev, etc.
> > >>
> > >>I recently posted a series of patches that allows lower-level comedi
> > >>drivers to supply separate hooks for auto-configuring PCI devices or
> > >>USB devices without abusing the old "manual configuration" code
> > >>paths, see <http://driverdev.linuxdriverproject.org/pipermail/devel/2012-March/025331.html>.
> > >>
> > >>The old loop that searches the PCI bus is still needed for the
> > >>"manual configuration" code path.
> > >
> > >So, now that I've applied your patches, this patch isn't needed anymore,
> > >right?  Or should it be reworked to use the new interfaces?
> > 
> > I think it's still needed as that the comedi_driver->attach() hook
> > is still needed to support manual configuration by the
> > COMEDI_DEVCONFIG ioctl, and the existing code really shouldn't be
> > looking in s626_pci_table[] as it's tagged __devinitconst.
> > 
> > I never saw any mismatched section warnings when I compiled a
> > linux-next kernel on my system with a full set of comedi drivers,
> > even with CONFIG_DEBUG_SECTION_MISMATCH configured.  I'm not sure
> > why not, unless it has something to do with me building the kernel
> > with a separate build directory.
> 
> Yes, I don't see the mismatch either, so I'm not going to apply this
> patch, sorry, as I don't see what it is trying to fix at all.
> 
> greg k-h

I am getting an error when doing a build of the latest linux-next
before it gets to modpost so I need to look at that, but when I do an
allyesconfig build on master I get the following:

WARNING: drivers/staging/comedi/drivers/s626.o(.text+0x2ec8): Section mismatch in reference from the function s626_attach() to the variable .devinit.rodata:s626_pci_table
The function s626_attach() references
the variable __devinitconst s626_pci_table.
This is often because s626_attach lacks a __devinitconst 
annotation or the annotation of s626_pci_table is wrong.

WARNING: drivers/staging/comedi/drivers/s626.o(.text+0x2ece): Section mismatch in reference from the function s626_attach() to the variable .devinit.rodata:s626_pci_table
The function s626_attach() references
the variable __devinitconst s626_pci_table.
This is often because s626_attach lacks a __devinitconst 
annotation or the annotation of s626_pci_table is wrong.

WARNING: drivers/staging/comedi/drivers/s626.o(.text+0x2ed7): Section mismatch in reference from the function s626_attach() to the variable .devinit.rodata:s626_pci_table
The function s626_attach() references
the variable __devinitconst s626_pci_table.
This is often because s626_attach lacks a __devinitconst 
annotation or the annotation of s626_pci_table is wrong.

WARNING: drivers/staging/comedi/drivers/s626.o(.text+0x2edd): Section mismatch in reference from the function s626_attach() to the variable .devinit.rodata:s626_pci_table
The function s626_attach() references
the variable __devinitconst s626_pci_table.
This is often because s626_attach lacks a __devinitconst 
annotation or the annotation of s626_pci_table is wrong.

Re: [PATCH v2] staging: comedi: resolve section mismatch in s626

[Greg KH]

On Thu, Apr 19, 2012 at 06:34:11PM -0700, Gerard Snitselaar wrote:
> I am getting an error when doing a build of the latest linux-next
> before it gets to modpost so I need to look at that, but when I do an
> allyesconfig build on master I get the following:
> 
> WARNING: drivers/staging/comedi/drivers/s626.o(.text+0x2ec8): Section mismatch in reference from the function s626_attach() to the variable .devinit.rodata:s626_pci_table
> The function s626_attach() references
> the variable __devinitconst s626_pci_table.
> This is often because s626_attach lacks a __devinitconst 
> annotation or the annotation of s626_pci_table is wrong.
> 
> WARNING: drivers/staging/comedi/drivers/s626.o(.text+0x2ece): Section mismatch in reference from the function s626_attach() to the variable .devinit.rodata:s626_pci_table
> The function s626_attach() references
> the variable __devinitconst s626_pci_table.
> This is often because s626_attach lacks a __devinitconst 
> annotation or the annotation of s626_pci_table is wrong.
> 
> WARNING: drivers/staging/comedi/drivers/s626.o(.text+0x2ed7): Section mismatch in reference from the function s626_attach() to the variable .devinit.rodata:s626_pci_table
> The function s626_attach() references
> the variable __devinitconst s626_pci_table.
> This is often because s626_attach lacks a __devinitconst 
> annotation or the annotation of s626_pci_table is wrong.
> 
> WARNING: drivers/staging/comedi/drivers/s626.o(.text+0x2edd): Section mismatch in reference from the function s626_attach() to the variable .devinit.rodata:s626_pci_table
> The function s626_attach() references
> the variable __devinitconst s626_pci_table.
> This is often because s626_attach lacks a __devinitconst 
> annotation or the annotation of s626_pci_table is wrong.

Odd, can you send me your .config file?

Also, I don't have this patch in my queue anymore, can someone please
resend it if it is to be applied?

greg k-h

Re: [PATCH v2 resend] staging: comedi: resolve section mismatch in s626

[Gerard Snitselaar]

s626_attach is referencing s626_pci_table which is annotated
__devinitconst. Put pci vendor/device ids in the s626_board struct and
use s626_boards instead similar to what is done in gsc_hpdi.

v2: I had moved the PCI id defines to s626.h earlier, but later
decided to leave them in s626.c and forgot to move them up above where
they are being used in s626_boards.

Signed-off-by: Gerard Snitselaar <dev@snitselaar.org>
---
 drivers/staging/comedi/drivers/s626.c |   27 +++++++++++++++++++--------
 1 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
index 23fc64b..e531f1c 100644
--- a/drivers/staging/comedi/drivers/s626.c
+++ b/drivers/staging/comedi/drivers/s626.c
@@ -83,8 +83,17 @@ MODULE_AUTHOR("Gianluca Palli <gpalli@deis.unibo.it>");
 MODULE_DESCRIPTION("Sensoray 626 Comedi driver module");
 MODULE_LICENSE("GPL");
 
+#define PCI_VENDOR_ID_S626 0x1131
+#define PCI_DEVICE_ID_S626 0x7146
+#define PCI_SUBVENDOR_ID_S626 0x6000
+#define PCI_SUBDEVICE_ID_S626 0x0272
+
 struct s626_board {
 	const char *name;
+	int vendor_id;
+	int device_id;
+	int subvendor_id;
+	int subdevice_id;
 	int ai_chans;
 	int ai_bits;
 	int ao_chans;
@@ -97,6 +106,10 @@ struct s626_board {
 static const struct s626_board s626_boards[] = {
 	{
 	 .name = "s626",
+	 .vendor_id = PCI_VENDOR_ID_S626,
+	 .device_id = PCI_DEVICE_ID_S626,
+	 .subvendor_id = PCI_SUBVENDOR_ID_S626,
+	 .subdevice_id = PCI_SUBDEVICE_ID_S626,
 	 .ai_chans = S626_ADC_CHANNELS,
 	 .ai_bits = 14,
 	 .ao_chans = S626_DAC_CHANNELS,
@@ -108,8 +121,6 @@ static const struct s626_board s626_boards[] = {
 };
 
 #define thisboard ((const struct s626_board *)dev->board_ptr)
-#define PCI_VENDOR_ID_S626 0x1131
-#define PCI_DEVICE_ID_S626 0x7146
 
 /*
  * For devices with vendor:device id == 0x1131:0x7146 you must specify
@@ -117,7 +128,7 @@ static const struct s626_board s626_boards[] = {
  * Philips SAA7146 media/dvb based cards.
  */
 static DEFINE_PCI_DEVICE_TABLE(s626_pci_table) = {
-	{PCI_VENDOR_ID_S626, PCI_DEVICE_ID_S626, 0x6000, 0x0272, 0, 0, 0},
+	{PCI_VENDOR_ID_S626, PCI_DEVICE_ID_S626, PCI_SUBVENDOR_ID_S626, PCI_SUBDEVICE_ID_S626, 0, 0, 0},
 	{0}
 };
 
@@ -554,17 +565,17 @@ static int s626_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 	resource_size_t resourceStart;
 	dma_addr_t appdma;
 	struct comedi_subdevice *s;
-	const struct pci_device_id *ids;
 	struct pci_dev *pdev = NULL;
 
 	if (alloc_private(dev, sizeof(struct s626_private)) < 0)
 		return -ENOMEM;
 
-	for (i = 0; i < (ARRAY_SIZE(s626_pci_table) - 1) && !pdev; i++) {
-		ids = &s626_pci_table[i];
+	for (i = 0; i < ARRAY_SIZE(s626_boards) && !pdev; i++) {
 		do {
-			pdev = pci_get_subsys(ids->vendor, ids->device,
-					      ids->subvendor, ids->subdevice,
+			pdev = pci_get_subsys(s626_boards[i].vendor_id,
+					      s626_boards[i].device_id,
+					      s626_boards[i].subvendor_id,
+					      s626_boards[i].subdevice_id,
 					      pdev);
 
 			if ((it->options[0] || it->options[1]) && pdev) {
-- 
1.7.7.6