| Message ID | 1495763586-27238-2-git-send-email-gwshan@linux.vnet.ibm.com |
|---|---|
| State | Rejected |
| Headers | show |
On Fri, 2017-05-26 at 11:53 +1000, Gavin Shan wrote: > This changes the capability data type from "void *" to "uint64_t" > so that it can hold pointer and data at the same time. No logicial > changes. I don't like that capability data stuff. It's only used by SR-IOV isn't it ? Why not just have a struct iov pointer in the pci_dev that you keep NULL when not used ? At least you get type checking... So I'd rather you remove the capability data completely. Also, we should just scan all caps at probe time and fill up the cache once and for all, so we don't have to use pci_set_cap() and worry as to whether a capability offset is cached or not. > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > --- > core/pci-iov.c | 2 +- > core/pci.c | 4 ++-- > include/pci.h | 8 ++++---- > 3 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/core/pci-iov.c b/core/pci-iov.c > index 14c810b..7b1d6dd 100644 > --- a/core/pci-iov.c > +++ b/core/pci-iov.c > @@ -253,5 +253,5 @@ void pci_init_iov_cap(struct phb *phb, struct pci_device *pd) > iov->pos = pos; > iov->enabled = false; > pci_iov_update_parameters(iov); > - pci_set_cap(pd, PCIECAP_ID_SRIOV, pos, iov, true); > + pci_set_cap(pd, PCIECAP_ID_SRIOV, pos, (uint64_t)iov, true); > } > diff --git a/core/pci.c b/core/pci.c > index 7ec8409..29c3df6 100644 > --- a/core/pci.c > +++ b/core/pci.c > @@ -162,7 +162,7 @@ static void pci_init_pcie_cap(struct phb *phb, struct pci_device *pd) > return; > } > > - pci_set_cap(pd, PCI_CFG_CAP_ID_EXP, ecap, NULL, false); > + pci_set_cap(pd, PCI_CFG_CAP_ID_EXP, ecap, 0ul, false); > > /* > * XXX We observe a problem on some PLX switches where one > @@ -198,7 +198,7 @@ static void pci_init_aer_cap(struct phb *phb, struct pci_device *pd) > > pos = pci_find_ecap(phb, pd->bdfn, PCIECAP_ID_AER, NULL); > if (pos > 0) > - pci_set_cap(pd, PCIECAP_ID_AER, pos, NULL, true); > + pci_set_cap(pd, PCIECAP_ID_AER, pos, 0ul, true); > } > > void pci_init_capabilities(struct phb *phb, struct pci_device *pd) > diff --git a/include/pci.h b/include/pci.h > index dc418a9..aaf11a6 100644 > --- a/include/pci.h > +++ b/include/pci.h > @@ -76,7 +76,7 @@ struct pci_device { > uint64_t cap_list; > struct { > uint32_t pos; > - void *data; > + uint64_t data; > } cap[64]; > uint32_t mps; /* Max payload size capability */ > > @@ -91,8 +91,8 @@ struct pci_device { > struct list_node link; > }; > > -static inline void pci_set_cap(struct pci_device *pd, int id, > - int pos, void *data, bool ext) > +static inline void pci_set_cap(struct pci_device *pd, uint32_t id, > + uint32_t pos, uint64_t data, bool ext) > { > if (!ext) { > pd->cap_list |= (0x1ul << id); > @@ -123,7 +123,7 @@ static inline int pci_cap(struct pci_device *pd, > return pd->cap[id + 32].pos; > } > > -static inline void *pci_cap_data(struct pci_device *pd, int id, bool ext) > +static inline uint64_t pci_cap_data(struct pci_device *pd, int id, bool ext) > { > if (!ext) > return pd->cap[id].data;
Gavin Shan <gwshan@linux.vnet.ibm.com> writes: > This changes the capability data type from "void *" to "uint64_t" > so that it can hold pointer and data at the same time. No logicial > changes. > > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > --- > core/pci-iov.c | 2 +- > core/pci.c | 4 ++-- > include/pci.h | 8 ++++---- > 3 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/core/pci-iov.c b/core/pci-iov.c > index 14c810b..7b1d6dd 100644 > --- a/core/pci-iov.c > +++ b/core/pci-iov.c > @@ -253,5 +253,5 @@ void pci_init_iov_cap(struct phb *phb, struct pci_device *pd) > iov->pos = pos; > iov->enabled = false; > pci_iov_update_parameters(iov); > - pci_set_cap(pd, PCIECAP_ID_SRIOV, pos, iov, true); > + pci_set_cap(pd, PCIECAP_ID_SRIOV, pos, (uint64_t)iov, true); I don't htink uint64_t is any better than void* for when it's being used as a pointer anyway. Is there a way to make this a bit clearer and more typesafe?
On Thu, Jun 15, 2017 at 03:43:18PM +1000, Benjamin Herrenschmidt wrote: >On Fri, 2017-05-26 at 11:53 +1000, Gavin Shan wrote: >> This changes the capability data type from "void *" to "uint64_t" >> so that it can hold pointer and data at the same time. No logicial >> changes. > >I don't like that capability data stuff. It's only used by SR-IOV >isn't it ? > >Why not just have a struct iov pointer in the pci_dev that you keep >NULL when not used ? At least you get type checking... > >So I'd rather you remove the capability data completely. > >Also, we should just scan all caps at probe time and fill up the >cache once and for all, so we don't have to use pci_set_cap() and >worry as to whether a capability offset is cached or not. > Ok. All suggestions will be included in v2. Cheers, Gavin >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >> --- >> core/pci-iov.c | 2 +- >> core/pci.c | 4 ++-- >> include/pci.h | 8 ++++---- >> 3 files changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/core/pci-iov.c b/core/pci-iov.c >> index 14c810b..7b1d6dd 100644 >> --- a/core/pci-iov.c >> +++ b/core/pci-iov.c >> @@ -253,5 +253,5 @@ void pci_init_iov_cap(struct phb *phb, struct pci_device *pd) >> iov->pos = pos; >> iov->enabled = false; >> pci_iov_update_parameters(iov); >> - pci_set_cap(pd, PCIECAP_ID_SRIOV, pos, iov, true); >> + pci_set_cap(pd, PCIECAP_ID_SRIOV, pos, (uint64_t)iov, true); >> } >> diff --git a/core/pci.c b/core/pci.c >> index 7ec8409..29c3df6 100644 >> --- a/core/pci.c >> +++ b/core/pci.c >> @@ -162,7 +162,7 @@ static void pci_init_pcie_cap(struct phb *phb, struct pci_device *pd) >> return; >> } >> >> - pci_set_cap(pd, PCI_CFG_CAP_ID_EXP, ecap, NULL, false); >> + pci_set_cap(pd, PCI_CFG_CAP_ID_EXP, ecap, 0ul, false); >> >> /* >> * XXX We observe a problem on some PLX switches where one >> @@ -198,7 +198,7 @@ static void pci_init_aer_cap(struct phb *phb, struct pci_device *pd) >> >> pos = pci_find_ecap(phb, pd->bdfn, PCIECAP_ID_AER, NULL); >> if (pos > 0) >> - pci_set_cap(pd, PCIECAP_ID_AER, pos, NULL, true); >> + pci_set_cap(pd, PCIECAP_ID_AER, pos, 0ul, true); >> } >> >> void pci_init_capabilities(struct phb *phb, struct pci_device *pd) >> diff --git a/include/pci.h b/include/pci.h >> index dc418a9..aaf11a6 100644 >> --- a/include/pci.h >> +++ b/include/pci.h >> @@ -76,7 +76,7 @@ struct pci_device { >> uint64_t cap_list; >> struct { >> uint32_t pos; >> - void *data; >> + uint64_t data; >> } cap[64]; >> uint32_t mps; /* Max payload size capability */ >> >> @@ -91,8 +91,8 @@ struct pci_device { >> struct list_node link; >> }; >> >> -static inline void pci_set_cap(struct pci_device *pd, int id, >> - int pos, void *data, bool ext) >> +static inline void pci_set_cap(struct pci_device *pd, uint32_t id, >> + uint32_t pos, uint64_t data, bool ext) >> { >> if (!ext) { >> pd->cap_list |= (0x1ul << id); >> @@ -123,7 +123,7 @@ static inline int pci_cap(struct pci_device *pd, >> return pd->cap[id + 32].pos; >> } >> >> -static inline void *pci_cap_data(struct pci_device *pd, int id, bool ext) >> +static inline uint64_t pci_cap_data(struct pci_device *pd, int id, bool ext) >> { >> if (!ext) >> return pd->cap[id].data; >
On Fri, Jun 16, 2017 at 03:16:08PM +1000, Stewart Smith wrote: >Gavin Shan <gwshan@linux.vnet.ibm.com> writes: >> This changes the capability data type from "void *" to "uint64_t" >> so that it can hold pointer and data at the same time. No logicial >> changes. >> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >> --- >> core/pci-iov.c | 2 +- >> core/pci.c | 4 ++-- >> include/pci.h | 8 ++++---- >> 3 files changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/core/pci-iov.c b/core/pci-iov.c >> index 14c810b..7b1d6dd 100644 >> --- a/core/pci-iov.c >> +++ b/core/pci-iov.c >> @@ -253,5 +253,5 @@ void pci_init_iov_cap(struct phb *phb, struct pci_device *pd) >> iov->pos = pos; >> iov->enabled = false; >> pci_iov_update_parameters(iov); >> - pci_set_cap(pd, PCIECAP_ID_SRIOV, pos, iov, true); >> + pci_set_cap(pd, PCIECAP_ID_SRIOV, pos, (uint64_t)iov, true); > >I don't htink uint64_t is any better than void* for when it's being used >as a pointer anyway. > >Is there a way to make this a bit clearer and more typesafe? > As Ben suggested, it's going to removed as it's used by SRIOV only. The IOV struct will be associated with the PF directly. Cheers, Gavin
diff --git a/core/pci-iov.c b/core/pci-iov.c index 14c810b..7b1d6dd 100644 --- a/core/pci-iov.c +++ b/core/pci-iov.c @@ -253,5 +253,5 @@ void pci_init_iov_cap(struct phb *phb, struct pci_device *pd) iov->pos = pos; iov->enabled = false; pci_iov_update_parameters(iov); - pci_set_cap(pd, PCIECAP_ID_SRIOV, pos, iov, true); + pci_set_cap(pd, PCIECAP_ID_SRIOV, pos, (uint64_t)iov, true); } diff --git a/core/pci.c b/core/pci.c index 7ec8409..29c3df6 100644 --- a/core/pci.c +++ b/core/pci.c @@ -162,7 +162,7 @@ static void pci_init_pcie_cap(struct phb *phb, struct pci_device *pd) return; } - pci_set_cap(pd, PCI_CFG_CAP_ID_EXP, ecap, NULL, false); + pci_set_cap(pd, PCI_CFG_CAP_ID_EXP, ecap, 0ul, false); /* * XXX We observe a problem on some PLX switches where one @@ -198,7 +198,7 @@ static void pci_init_aer_cap(struct phb *phb, struct pci_device *pd) pos = pci_find_ecap(phb, pd->bdfn, PCIECAP_ID_AER, NULL); if (pos > 0) - pci_set_cap(pd, PCIECAP_ID_AER, pos, NULL, true); + pci_set_cap(pd, PCIECAP_ID_AER, pos, 0ul, true); } void pci_init_capabilities(struct phb *phb, struct pci_device *pd) diff --git a/include/pci.h b/include/pci.h index dc418a9..aaf11a6 100644 --- a/include/pci.h +++ b/include/pci.h @@ -76,7 +76,7 @@ struct pci_device { uint64_t cap_list; struct { uint32_t pos; - void *data; + uint64_t data; } cap[64]; uint32_t mps; /* Max payload size capability */ @@ -91,8 +91,8 @@ struct pci_device { struct list_node link; }; -static inline void pci_set_cap(struct pci_device *pd, int id, - int pos, void *data, bool ext) +static inline void pci_set_cap(struct pci_device *pd, uint32_t id, + uint32_t pos, uint64_t data, bool ext) { if (!ext) { pd->cap_list |= (0x1ul << id); @@ -123,7 +123,7 @@ static inline int pci_cap(struct pci_device *pd, return pd->cap[id + 32].pos; } -static inline void *pci_cap_data(struct pci_device *pd, int id, bool ext) +static inline uint64_t pci_cap_data(struct pci_device *pd, int id, bool ext) { if (!ext) return pd->cap[id].data;
This changes the capability data type from "void *" to "uint64_t" so that it can hold pointer and data at the same time. No logicial changes. Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> --- core/pci-iov.c | 2 +- core/pci.c | 4 ++-- include/pci.h | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-)