Adding PNP info to puc, bce #136

lakhanshiva · 2018-03-29T16:32:40Z

lakhanshiva

Need to look at struct definition for devices array to determine correct format string

@cemeyer commented - For this driver, the device table data itself will need some modification because the table assumes that a subvendor ANY match means the subdevice id is ignored, and so 0 is often used for table subdevice id when subvendor is ANY. I think many subdevice id zeros in this driver need to be converted to 0xffff.
@cemeyer - The puc_pci_devices array is a puc_cfg struct defined as extern const struct puc_cfg puc_pci_devices[];

The puc_cfg struct is
struct puc_cfg {
uint16_t vendor;
uint16_t device;
uint16_t subvendor;
uint16_t subdevice;
const char desc;
int clock;
int8_t ports;
int8_t rid; / Rid of first port /
int8_t d_rid; / Delta rid of next ports /
int8_t d_ofs; / Delta offset of next ports */
puc_config_f *config_function;
};

cemeyer · 2018-03-29T17:09:46Z

sys/dev/puc/puc_pci.c

@@ -198,3 +198,4 @@ static driver_t puc_pci_driver = {
 };

 DRIVER_MODULE(puc, pci, puc_pci_driver, puc_devclass, 0, 0);
+MODULE_PNP_INFO("U16:vendor;U16:device;V16:subvendor;V16:subdevice;D:#", pci, puc, puc_pci_devices, sizeof(puc_pci_devices[0]), nitems(puc_pci_devices) - 1);


This change fails to adapt the puc_pci_devices table as described. Additionally, long lines should be wrapped — see style(9).

I'd be more inclined to, in this case, leave out the subvendor and subdevice. They aren't needed to differentiate which cards the driver might load for, just to configure parameters of those cards. Therefore, it's easier to just ignore them (use U16:#; for each of the fields to ignore). That also solves the style issue.

Actually cem, I think we should just ignore them. They don't change which driver gets the card, but do change how things are configured. I'd rather under-specify a smidge here than do the churn on the table.
"U16:vendor;U16:device;U16:#;U16:#;D:#;" seems like a much better choice for the descriptor string in this case.

That's a good point. Matching on vendor+devid is probably good enough -- even if it is false positive and some other driver is needed for the specific subvendor/subdevice, the harm is low -- one extra driver is loaded.

Should they be anonymous '#'s, or just remove them (and 'D:#') from the format string entirely?

I think they should be anonymous #'s because I like to keep the description if possible. I'm still on the fence on its usefulness, and until I jump one way or the other, I'd like to tag as much as possible.

I placed the anonymous #'s for now. For subvendor and subdevice, is it right to use U16 instead of V16 (which cemeyer has suggested) ?

The puc_pci_devices array is a puc_cfg struct defined as extern const struct puc_cfg puc_pci_devices[];

The puc_cfg struct is
struct puc_cfg {
uint16_t vendor;
uint16_t device;
uint16_t subvendor;
uint16_t subdevice;
const char desc;
int clock;
int8_t ports;
int8_t rid; / Rid of first port /
int8_t d_rid; / Delta rid of next ports /
int8_t d_ofs; / Delta offset of next ports */
puc_config_f *config_function;
};

How can i adapt this PNP_INFO to puc_pci_devices table ?

The PNP_INFO need only describe the first few items in the table. Since we encode a count of items, and their size as well, we need not describe each entry in the table. That's why I recommended only describing the first few, up through the desc.

bsdimp · 2018-03-29T17:27:04Z

sys/dev/bce/if_bce.c

@@ -529,7 +529,7 @@ MODULE_DEPEND(bce, miibus, 1, 1, 1);

 DRIVER_MODULE(bce, pci, bce_driver, bce_devclass, NULL, NULL);
 DRIVER_MODULE(miibus, bce, miibus_driver, miibus_devclass, NULL, NULL);
-
+MODULE_PNP_INFO("U16:vendor;U16:device;V16:subvendor;V16:subdevice;D:#", pci, bce, dce_devs, sizeof(bce_devs[0]), nitems(bce_devs) - 1);


I think this line is now also too long.

cemeyer · 2018-03-30T15:25:01Z

For subvendor and subdevice, is it right to use U16 instead of V16 (which cemeyer has suggested) ?

Doesn't matter if they will be ignored ('#' name). They have the same width — U vs V just affects the match behavior.

bsdimp · 2018-04-01T23:17:23Z

sys/dev/bce/if_bce.c

@@ -529,7 +529,8 @@ MODULE_DEPEND(bce, miibus, 1, 1, 1);

 DRIVER_MODULE(bce, pci, bce_driver, bce_devclass, NULL, NULL);
 DRIVER_MODULE(miibus, bce, miibus_driver, miibus_devclass, NULL, NULL);
-
+MODULE_PNP_INFO("U16:vendor;U16:device;U16:#;U16:#;D:#", pci, bce,
+		dce_devs, sizeof(bce_devs[0]), nitems(bce_devs) - 1);


Not quite right: dce_devs here I think should be bce_devs.

bsdimp · 2018-04-01T23:17:48Z

sys/dev/puc/puc_pci.c

@@ -198,3 +198,6 @@ static driver_t puc_pci_driver = {
 };

 DRIVER_MODULE(puc, pci, puc_pci_driver, puc_devclass, 0, 0);
+MODULE_PNP_INFO("U16:vendor;U16:device;U16:#;U16:#;D:#", pci, puc,
+		puc_pci_devices, sizeof(puc_pci_devices[0]),
+		nitems(puc_pci_devices) - 1);


This doesn't compile because the size of puc_pci_devices isn't known here.

I see, that might be because this table is not present directly in the same file. It is present in another one. What could be done here to correct this ?

I think you'll need to rearrange the code a little so that puc_pci_devices is included in pcu_pci.c somehow. I'd recommend doing it with a #include of the current file that has it since I believe that file is shared with NetBSD.

Included pucdata.c which contains puc_pci_devices

missed the reply to this until now...

This looks like it could work, but pucdata.c is already built in the module and specified in the kernel config files (see sys/conf/files for lines containing pucdata.c). So, adjustments in those areas are needed:

diff --git a/sys/conf/files b/sys/conf/files
index d680535ae0b9..b2da16980cee 100644
--- a/sys/conf/files
+++ b/sys/conf/files
@@ -2771,7 +2771,6 @@ dev/puc/puc.c optional puc
dev/puc/puc_cfg.c optional puc
dev/puc/puc_pccard.c optional puc pccard
dev/puc/puc_pci.c optional puc pci
-dev/puc/pucdata.c optional puc pci
dev/quicc/quicc_core.c optional quicc
dev/ral/rt2560.c optional ral
dev/ral/rt2661.c optional ral
diff --git a/sys/modules/puc/Makefile b/sys/modules/puc/Makefile
index 042ae102abef..882e33343067 100644
--- a/sys/modules/puc/Makefile
+++ b/sys/modules/puc/Makefile
@@ -4,7 +4,7 @@
.PATH: ${SRCTOP}/sys/dev/puc

KMOD= puc
-SRCS= puc.c puc_cfg.c puc_pci.c puc_pccard.c pucdata.c
+SRCS= puc.c puc_cfg.c puc_pci.c puc_pccard.c
SRCS+= bus_if.h device_if.h serdev_if.c serdev_if.h
card_if.h pci_if.h

If you do any future pull requests, please make sure they compile.

Submitted by: Lakhan Shiva Kamireddy Pull Request: #136 git-svn-id: svn+ssh://svn.freebsd.org/base/head@332651 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

Adjust sys/conf/files and sys/modules/puc/Makefile to omit pucdata.c now tht it's included by puc_pci.c. Submitted by: Lakhan Shiva Kamireddy (with build fixes by me) Pull Request: #136 git-svn-id: svn+ssh://svn.freebsd.org/base/head@332652 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

Submitted by: Lakhan Shiva Kamireddy Pull Request: #136

Adjust sys/conf/files and sys/modules/puc/Makefile to omit pucdata.c now tht it's included by puc_pci.c. Submitted by: Lakhan Shiva Kamireddy (with build fixes by me) Pull Request: #136

Submitted by: Lakhan Shiva Kamireddy Pull Request: freebsd#136 git-svn-id: https://svn.freebsd.org/base/head@332651 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

Adjust sys/conf/files and sys/modules/puc/Makefile to omit pucdata.c now tht it's included by puc_pci.c. Submitted by: Lakhan Shiva Kamireddy (with build fixes by me) Pull Request: freebsd#136 git-svn-id: https://svn.freebsd.org/base/head@332652 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

Submitted by: Lakhan Shiva Kamireddy Pull Request: freebsd#136 git-svn-id: svn+ssh://svn.freebsd.org/base/head@332651 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

Adjust sys/conf/files and sys/modules/puc/Makefile to omit pucdata.c now tht it's included by puc_pci.c. Submitted by: Lakhan Shiva Kamireddy (with build fixes by me) Pull Request: freebsd#136 git-svn-id: svn+ssh://svn.freebsd.org/base/head@332652 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

lakhanshiva · 2018-04-24T05:20:18Z

There was a commit by mat813 https://github.com/mat813/freebsd/commit/ d6c76a7 , now removing the pucdata.c from *sys/conf/files* and *sys/modules/puc/Makefile* Also, is there any guide online for the process of making a Pull request to FreeBSD code ? Should i just run the command make to compile ?

…

On Tue, Apr 17, 2018 at 10:13 PM, Warner Losh ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In sys/dev/puc/puc_pci.c <#136 (comment)>: > @@ -198,3 +198,6 @@ static driver_t puc_pci_driver = { }; DRIVER_MODULE(puc, pci, puc_pci_driver, puc_devclass, 0, 0); +MODULE_PNP_INFO("U16:vendor;U16:device;U16:#;U16:#;D:#", pci, puc, + puc_pci_devices, sizeof(puc_pci_devices[0]), + nitems(puc_pci_devices) - 1); missed the reply to this until now... This looks like it could work, but pucdata.c is already built in the module and specified in the kernel config files (see sys/conf/files for lines containing pucdata.c). So, adjustments in those areas are needed: diff --git a/sys/conf/files b/sys/conf/files index d680535ae0b9..b2da16980cee 100644 --- a/sys/conf/files +++ b/sys/conf/files @@ -2771,7 +2771,6 @@ dev/puc/puc.c optional puc dev/puc/puc_cfg.c optional puc dev/puc/puc_pccard.c optional puc pccard dev/puc/puc_pci.c optional puc pci -dev/puc/pucdata.c optional puc pci dev/quicc/quicc_core.c optional quicc dev/ral/rt2560.c optional ral dev/ral/rt2661.c optional ral diff --git a/sys/modules/puc/Makefile b/sys/modules/puc/Makefile index 042ae102abef..882e33343067 100644 --- a/sys/modules/puc/Makefile +++ b/sys/modules/puc/Makefile @@ -4,7 +4,7 @@ .PATH: ${SRCTOP}/sys/dev/puc KMOD= puc -SRCS= puc.c puc_cfg.c puc_pci.c puc_pccard.c pucdata.c +SRCS= puc.c puc_cfg.c puc_pci.c puc_pccard.c SRCS+= bus_if.h device_if.h serdev_if.c serdev_if.h card_if.h pci_if.h If you do any future pull requests, please make sure they compile. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#136 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEiqmAfQCN3wQmeaBIrLPxeHZHWA0wPpks5tphvPgaJpZM4TAoOl> .

Submitted by: Lakhan Shiva Kamireddy Pull Request: freebsd/freebsd-src#136

Adjust sys/conf/files and sys/modules/puc/Makefile to omit pucdata.c now tht it's included by puc_pci.c. Submitted by: Lakhan Shiva Kamireddy (with build fixes by me) Pull Request: freebsd/freebsd-src#136

lakhanshiva commented Mar 29, 2018

View reviewed changes

cemeyer suggested changes Mar 29, 2018

View reviewed changes

bsdimp reviewed Mar 29, 2018

View reviewed changes

bsdimp reviewed Apr 1, 2018

View reviewed changes

uqs pushed a commit that referenced this pull request Apr 17, 2018

Add PNP info to the bce driver.

4d6c556

Submitted by: Lakhan Shiva Kamireddy Pull Request: #136 git-svn-id: svn+ssh://svn.freebsd.org/base/head@332651 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

uqs pushed a commit that referenced this pull request Apr 17, 2018

Add PNP info to the bce driver.

b0ccb12

Submitted by: Lakhan Shiva Kamireddy Pull Request: #136

mat813 pushed a commit to mat813/freebsd that referenced this pull request Apr 18, 2018

Add PNP info to the bce driver.

6923458

Submitted by: Lakhan Shiva Kamireddy Pull Request: freebsd#136 git-svn-id: https://svn.freebsd.org/base/head@332651 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

bdrewery pushed a commit to bdrewery/freebsd that referenced this pull request Apr 23, 2018

Add PNP info to the bce driver.

8ba086d

Submitted by: Lakhan Shiva Kamireddy Pull Request: freebsd#136 git-svn-id: svn+ssh://svn.freebsd.org/base/head@332651 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

lakhanshiva closed this May 3, 2018

lakhanshiva force-pushed the master branch from 5f544d0 to 2e45301 Compare May 3, 2018 05:44

brooksdavis pushed a commit to CTSRD-CHERI/cheribsd that referenced this pull request Aug 24, 2018

Add PNP info to the bce driver.

d75e8df

Submitted by: Lakhan Shiva Kamireddy Pull Request: freebsd/freebsd-src#136

emaste added the merged label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding PNP info to puc, bce #136

Adding PNP info to puc, bce #136

lakhanshiva commented Mar 29, 2018

lakhanshiva left a comment •

edited

Loading

cemeyer Mar 29, 2018

bsdimp Mar 29, 2018

bsdimp Mar 29, 2018

cemeyer Mar 29, 2018

bsdimp Mar 29, 2018

lakhanshiva Mar 30, 2018

bsdimp Mar 30, 2018 •

edited

Loading

bsdimp Mar 29, 2018

cemeyer commented Mar 30, 2018

bsdimp Apr 1, 2018

bsdimp Apr 1, 2018

lakhanshiva Apr 2, 2018

bsdimp Apr 2, 2018

lakhanshiva Apr 5, 2018

bsdimp Apr 17, 2018

lakhanshiva commented Apr 24, 2018 via email

Adding PNP info to puc, bce #136

Adding PNP info to puc, bce #136

Conversation

lakhanshiva commented Mar 29, 2018

lakhanshiva left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsdimp Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cemeyer commented Mar 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lakhanshiva commented Apr 24, 2018 via email

lakhanshiva left a comment •

edited

Loading

bsdimp Mar 30, 2018 •

edited

Loading