* [igt-dev] [PATCH i-g-t v3 0/5] Cold Reset IGT Test @ 2022-11-23 8:52 Anshuman Gupta 2022-11-23 8:52 ` [igt-dev] [PATCH i-g-t v3 1/5] lib/igt_pci: helpers to get PCI capabilities offset Anshuman Gupta 0 siblings, 1 reply; 8+ messages in thread From: Anshuman Gupta @ 2022-11-23 8:52 UTC (permalink / raw) To: igt-dev; +Cc: badal.nilawar, rodrigo.vivi Adding Cold Reset IGT support v3: - Address the review comment from Aravind. Anshuman Gupta (5): lib/igt_pci: helpers to get PCI capabilities offset lib/igt_pci: Add PCIe slot cap lib/igt_pm: Refactor get firmware_node fd test/device_reset: Refactor initiate_device_reset tests/device_reset: Add cold reset IGT test lib/igt_pci.c | 44 ++++++++++++ lib/igt_pci.h | 35 +++++++++ lib/igt_pm.c | 76 +++++++++++++++++--- lib/igt_pm.h | 1 + lib/meson.build | 1 + tests/device_reset.c | 167 +++++++++++++++++++++++++++++++++++++++---- 6 files changed, 300 insertions(+), 24 deletions(-) create mode 100644 lib/igt_pci.c create mode 100644 lib/igt_pci.h -- 2.25.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [igt-dev] [PATCH i-g-t v3 1/5] lib/igt_pci: helpers to get PCI capabilities offset 2022-11-23 8:52 [igt-dev] [PATCH i-g-t v3 0/5] Cold Reset IGT Test Anshuman Gupta @ 2022-11-23 8:52 ` Anshuman Gupta 0 siblings, 0 replies; 8+ messages in thread From: Anshuman Gupta @ 2022-11-23 8:52 UTC (permalink / raw) To: igt-dev; +Cc: badal.nilawar, rodrigo.vivi [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset=yes, Size: 2900 bytes --] Added helper functions to search for PCI capabilities with help of libpciaccess library. Cc: Ashutosh Dixit <ashutosh.dixit@intel.com> Co-developed-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> --- lib/igt_pci.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ lib/igt_pci.h | 32 ++++++++++++++++++++++++++++++++ lib/meson.build | 1 + 3 files changed, 77 insertions(+) create mode 100644 lib/igt_pci.c create mode 100644 lib/igt_pci.h diff --git a/lib/igt_pci.c b/lib/igt_pci.c new file mode 100644 index 0000000000..bc0369341d --- /dev/null +++ b/lib/igt_pci.c @@ -0,0 +1,44 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2022 Intel Corporation + */ + +#include <pciaccess.h> +#include "igt_pci.h" + +/** + * find_pci_cap_offset: + * @dev: pci device + * @cap_id: searched capability id, 0 means any capability + * @start_offset: offset in config space from which we start searching + * + * return: + * -1 on config read error, 0 if capability is not found, + * otherwise offset at which capability with cap_id is found + */ +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id, + int start_offset) +{ + uint8_t offset; + uint16_t cap_header; + int loop = (PCI_CFG_SPACE_SIZE - PCI_TYPE0_1_HEADER_SIZE) + / sizeof(cap_header); + + if (pci_device_cfg_read_u8(dev, &offset, start_offset)) + return -1; + + while (loop--) { + if (offset < PCI_TYPE0_1_HEADER_SIZE) + break; + + if (pci_device_cfg_read_u16(dev, &cap_header, (offset & 0xFC))) + return -1; + + if (!cap_id || cap_id == (cap_header & 0xFF)) + return offset; + + offset = cap_header >> 8; + } + + return 0; +} diff --git a/lib/igt_pci.h b/lib/igt_pci.h new file mode 100644 index 0000000000..68afd2dacb --- /dev/null +++ b/lib/igt_pci.h @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2022 Intel Corporation + */ + +#ifndef __IGT_PCI_H__ +#define __IGT_PCI_H__ + +#include <stdint.h> +#include <endian.h> + +/* forward declaration */ +struct pci_device; + +#define PCI_TYPE0_1_HEADER_SIZE 0x40 +#define PCI_CAPS_START 0x34 +#define PCI_CFG_SPACE_SIZE 0x100 +#define PCIE_CFG_SPACE_SIZE 0x1000 + +enum pci_cap_id { + PCI_EXPRESS_CAP_ID = 0x10 +}; + +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id, + int start_offset); + +static inline int find_pci_cap_offset(struct pci_device *dev, enum pci_cap_id cap_id) +{ + return find_pci_cap_offset_at(dev, cap_id, PCI_CAPS_START); +} + +#endif diff --git a/lib/meson.build b/lib/meson.build index cef2d0ff3d..e0fb7ddfed 100644 --- a/lib/meson.build +++ b/lib/meson.build @@ -33,6 +33,7 @@ lib_sources = [ 'igt_pipe_crc.c', 'igt_power.c', 'igt_primes.c', + 'igt_pci.c', 'igt_rand.c', 'igt_stats.c', 'igt_syncobj.c', -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [igt-dev] [PATCH i-g-t v3 0/5] Cold Reset IGT Test @ 2022-11-23 8:54 Anshuman Gupta 2022-11-23 8:54 ` [igt-dev] [PATCH i-g-t v3 1/5] lib/igt_pci: helpers to get PCI capabilities offset Anshuman Gupta 0 siblings, 1 reply; 8+ messages in thread From: Anshuman Gupta @ 2022-11-23 8:54 UTC (permalink / raw) To: igt-dev; +Cc: badal.nilawar, rodrigo.vivi Adding Cold Reset IGT support v3: - Address the review comment from Aravind. Anshuman Gupta (5): lib/igt_pci: helpers to get PCI capabilities offset lib/igt_pci: Add PCIe slot cap lib/igt_pm: Refactor get firmware_node fd test/device_reset: Refactor initiate_device_reset tests/device_reset: Add cold reset IGT test lib/igt_pci.c | 44 ++++++++++++ lib/igt_pci.h | 35 +++++++++ lib/igt_pm.c | 76 +++++++++++++++++--- lib/igt_pm.h | 1 + lib/meson.build | 1 + tests/device_reset.c | 167 +++++++++++++++++++++++++++++++++++++++---- 6 files changed, 300 insertions(+), 24 deletions(-) create mode 100644 lib/igt_pci.c create mode 100644 lib/igt_pci.h -- 2.25.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [igt-dev] [PATCH i-g-t v3 1/5] lib/igt_pci: helpers to get PCI capabilities offset 2022-11-23 8:54 [igt-dev] [PATCH i-g-t v3 0/5] Cold Reset IGT Test Anshuman Gupta @ 2022-11-23 8:54 ` Anshuman Gupta 2022-11-29 12:00 ` Nilawar, Badal 0 siblings, 1 reply; 8+ messages in thread From: Anshuman Gupta @ 2022-11-23 8:54 UTC (permalink / raw) To: igt-dev; +Cc: badal.nilawar, rodrigo.vivi [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset=yes, Size: 2900 bytes --] Added helper functions to search for PCI capabilities with help of libpciaccess library. Cc: Ashutosh Dixit <ashutosh.dixit@intel.com> Co-developed-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> --- lib/igt_pci.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ lib/igt_pci.h | 32 ++++++++++++++++++++++++++++++++ lib/meson.build | 1 + 3 files changed, 77 insertions(+) create mode 100644 lib/igt_pci.c create mode 100644 lib/igt_pci.h diff --git a/lib/igt_pci.c b/lib/igt_pci.c new file mode 100644 index 0000000000..bc0369341d --- /dev/null +++ b/lib/igt_pci.c @@ -0,0 +1,44 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2022 Intel Corporation + */ + +#include <pciaccess.h> +#include "igt_pci.h" + +/** + * find_pci_cap_offset: + * @dev: pci device + * @cap_id: searched capability id, 0 means any capability + * @start_offset: offset in config space from which we start searching + * + * return: + * -1 on config read error, 0 if capability is not found, + * otherwise offset at which capability with cap_id is found + */ +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id, + int start_offset) +{ + uint8_t offset; + uint16_t cap_header; + int loop = (PCI_CFG_SPACE_SIZE - PCI_TYPE0_1_HEADER_SIZE) + / sizeof(cap_header); + + if (pci_device_cfg_read_u8(dev, &offset, start_offset)) + return -1; + + while (loop--) { + if (offset < PCI_TYPE0_1_HEADER_SIZE) + break; + + if (pci_device_cfg_read_u16(dev, &cap_header, (offset & 0xFC))) + return -1; + + if (!cap_id || cap_id == (cap_header & 0xFF)) + return offset; + + offset = cap_header >> 8; + } + + return 0; +} diff --git a/lib/igt_pci.h b/lib/igt_pci.h new file mode 100644 index 0000000000..68afd2dacb --- /dev/null +++ b/lib/igt_pci.h @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2022 Intel Corporation + */ + +#ifndef __IGT_PCI_H__ +#define __IGT_PCI_H__ + +#include <stdint.h> +#include <endian.h> + +/* forward declaration */ +struct pci_device; + +#define PCI_TYPE0_1_HEADER_SIZE 0x40 +#define PCI_CAPS_START 0x34 +#define PCI_CFG_SPACE_SIZE 0x100 +#define PCIE_CFG_SPACE_SIZE 0x1000 + +enum pci_cap_id { + PCI_EXPRESS_CAP_ID = 0x10 +}; + +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id, + int start_offset); + +static inline int find_pci_cap_offset(struct pci_device *dev, enum pci_cap_id cap_id) +{ + return find_pci_cap_offset_at(dev, cap_id, PCI_CAPS_START); +} + +#endif diff --git a/lib/meson.build b/lib/meson.build index cef2d0ff3d..e0fb7ddfed 100644 --- a/lib/meson.build +++ b/lib/meson.build @@ -33,6 +33,7 @@ lib_sources = [ 'igt_pipe_crc.c', 'igt_power.c', 'igt_primes.c', + 'igt_pci.c', 'igt_rand.c', 'igt_stats.c', 'igt_syncobj.c', -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v3 1/5] lib/igt_pci: helpers to get PCI capabilities offset 2022-11-23 8:54 ` [igt-dev] [PATCH i-g-t v3 1/5] lib/igt_pci: helpers to get PCI capabilities offset Anshuman Gupta @ 2022-11-29 12:00 ` Nilawar, Badal 2022-11-30 12:43 ` Kamil Konieczny 0 siblings, 1 reply; 8+ messages in thread From: Nilawar, Badal @ 2022-11-29 12:00 UTC (permalink / raw) To: Anshuman Gupta, igt-dev; +Cc: rodrigo.vivi Hi Anshuman, On 23-11-2022 14:24, Anshuman Gupta wrote: > Added helper functions to search for PCI capabilities > with help of libpciaccess library. > > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com> > Co-developed-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com> > Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > --- > lib/igt_pci.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > lib/igt_pci.h | 32 ++++++++++++++++++++++++++++++++ > lib/meson.build | 1 + > 3 files changed, 77 insertions(+) > create mode 100644 lib/igt_pci.c > create mode 100644 lib/igt_pci.h > > diff --git a/lib/igt_pci.c b/lib/igt_pci.c > new file mode 100644 > index 0000000000..bc0369341d > --- /dev/null > +++ b/lib/igt_pci.c > @@ -0,0 +1,44 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright © 2022 Intel Corporation > + */ > + > +#include <pciaccess.h> > +#include "igt_pci.h" > + > +/** > + * find_pci_cap_offset: > + * @dev: pci device > + * @cap_id: searched capability id, 0 means any capability > + * @start_offset: offset in config space from which we start searching > + * > + * return: > + * -1 on config read error, 0 if capability is not found, > + * otherwise offset at which capability with cap_id is found > + */ > +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id, > + int start_offset) > +{ > + uint8_t offset; > + uint16_t cap_header; > + int loop = (PCI_CFG_SPACE_SIZE - PCI_TYPE0_1_HEADER_SIZE) > + / sizeof(cap_header); > + > + if (pci_device_cfg_read_u8(dev, &offset, start_offset)) > + return -1; > + > + while (loop--) { > + if (offset < PCI_TYPE0_1_HEADER_SIZE) > + break; > + > + if (pci_device_cfg_read_u16(dev, &cap_header, (offset & 0xFC))) > + return -1; > + > + if (!cap_id || cap_id == (cap_header & 0xFF)) > + return offset; > + > + offset = cap_header >> 8; For the last capability, next capability address will always be 0. So above instead of using while(loop--) we can use while(offset). Regards, Badal > + } > + > + return 0; > +} > diff --git a/lib/igt_pci.h b/lib/igt_pci.h > new file mode 100644 > index 0000000000..68afd2dacb > --- /dev/null > +++ b/lib/igt_pci.h > @@ -0,0 +1,32 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright © 2022 Intel Corporation > + */ > + > +#ifndef __IGT_PCI_H__ > +#define __IGT_PCI_H__ > + > +#include <stdint.h> > +#include <endian.h> > + > +/* forward declaration */ > +struct pci_device; > + > +#define PCI_TYPE0_1_HEADER_SIZE 0x40 > +#define PCI_CAPS_START 0x34 > +#define PCI_CFG_SPACE_SIZE 0x100 > +#define PCIE_CFG_SPACE_SIZE 0x1000 > + > +enum pci_cap_id { > + PCI_EXPRESS_CAP_ID = 0x10 > +}; > + > +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id, > + int start_offset); > + > +static inline int find_pci_cap_offset(struct pci_device *dev, enum pci_cap_id cap_id) > +{ > + return find_pci_cap_offset_at(dev, cap_id, PCI_CAPS_START); > +} > + > +#endif > diff --git a/lib/meson.build b/lib/meson.build > index cef2d0ff3d..e0fb7ddfed 100644 > --- a/lib/meson.build > +++ b/lib/meson.build > @@ -33,6 +33,7 @@ lib_sources = [ > 'igt_pipe_crc.c', > 'igt_power.c', > 'igt_primes.c', > + 'igt_pci.c', > 'igt_rand.c', > 'igt_stats.c', > 'igt_syncobj.c', ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v3 1/5] lib/igt_pci: helpers to get PCI capabilities offset 2022-11-29 12:00 ` Nilawar, Badal @ 2022-11-30 12:43 ` Kamil Konieczny 2022-11-30 12:56 ` Nilawar, Badal 2022-12-07 16:58 ` Bernatowicz, Marcin 0 siblings, 2 replies; 8+ messages in thread From: Kamil Konieczny @ 2022-11-30 12:43 UTC (permalink / raw) To: igt-dev; +Cc: Badal Nilawar, rodrigo.vivi Hi, On 2022-11-29 at 17:30:54 +0530, Nilawar, Badal wrote: > Hi Anshuman, > > On 23-11-2022 14:24, Anshuman Gupta wrote: > > Added helper functions to search for PCI capabilities > > with help of libpciaccess library. > > > > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com> > > Co-developed-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com> > > Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com> > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > > --- > > lib/igt_pci.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > > lib/igt_pci.h | 32 ++++++++++++++++++++++++++++++++ > > lib/meson.build | 1 + > > 3 files changed, 77 insertions(+) > > create mode 100644 lib/igt_pci.c > > create mode 100644 lib/igt_pci.h > > > > diff --git a/lib/igt_pci.c b/lib/igt_pci.c > > new file mode 100644 > > index 0000000000..bc0369341d > > --- /dev/null > > +++ b/lib/igt_pci.c > > @@ -0,0 +1,44 @@ > > +// SPDX-License-Identifier: MIT > > +/* > > + * Copyright © 2022 Intel Corporation > > + */ > > + > > +#include <pciaccess.h> > > +#include "igt_pci.h" > > + > > +/** > > + * find_pci_cap_offset: > > + * @dev: pci device > > + * @cap_id: searched capability id, 0 means any capability > > + * @start_offset: offset in config space from which we start searching > > + * > > + * return: > > + * -1 on config read error, 0 if capability is not found, > > + * otherwise offset at which capability with cap_id is found > > + */ > > +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id, > > + int start_offset) > > +{ > > + uint8_t offset; > > + uint16_t cap_header; > > + int loop = (PCI_CFG_SPACE_SIZE - PCI_TYPE0_1_HEADER_SIZE) > > + / sizeof(cap_header); > > + > > + if (pci_device_cfg_read_u8(dev, &offset, start_offset)) > > + return -1; > > + > > + while (loop--) { > > + if (offset < PCI_TYPE0_1_HEADER_SIZE) > > + break; > > + > > + if (pci_device_cfg_read_u16(dev, &cap_header, (offset & 0xFC))) > > + return -1; > > + > > + if (!cap_id || cap_id == (cap_header & 0xFF)) > > + return offset; > > + > > + offset = cap_header >> 8; > For the last capability, next capability address will always be 0. So above > instead of using while(loop--) we can use while(offset). > > Regards, > Badal This way we are guarded for endless looping, btw check for zero is just at loop enter (first if). > > + } > > + I would like to see additional check here: if (loop <= 0 && offset) return -1; or maybe assert here ? > > + return 0; > > +} > > diff --git a/lib/igt_pci.h b/lib/igt_pci.h > > new file mode 100644 > > index 0000000000..68afd2dacb > > --- /dev/null > > +++ b/lib/igt_pci.h > > @@ -0,0 +1,32 @@ > > +// SPDX-License-Identifier: MIT > > +/* > > + * Copyright © 2022 Intel Corporation > > + */ > > + > > +#ifndef __IGT_PCI_H__ > > +#define __IGT_PCI_H__ > > + > > +#include <stdint.h> > > +#include <endian.h> > > + > > +/* forward declaration */ > > +struct pci_device; > > + > > +#define PCI_TYPE0_1_HEADER_SIZE 0x40 > > +#define PCI_CAPS_START 0x34 > > +#define PCI_CFG_SPACE_SIZE 0x100 > > +#define PCIE_CFG_SPACE_SIZE 0x1000 ------------ ^ Remove this, it is unused. > > + > > +enum pci_cap_id { > > + PCI_EXPRESS_CAP_ID = 0x10 > > +}; > > + > > +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id, > > + int start_offset); > > + > > +static inline int find_pci_cap_offset(struct pci_device *dev, enum pci_cap_id cap_id) ---- ^ ---- ^ Remove this, it is work for optimizitaion in compiler, no need for it here. Btw do you expect user to use both functions ? +cc Marcin Regards, Kamil > > +{ > > + return find_pci_cap_offset_at(dev, cap_id, PCI_CAPS_START); > > +} > > + > > +#endif > > diff --git a/lib/meson.build b/lib/meson.build > > index cef2d0ff3d..e0fb7ddfed 100644 > > --- a/lib/meson.build > > +++ b/lib/meson.build > > @@ -33,6 +33,7 @@ lib_sources = [ > > 'igt_pipe_crc.c', > > 'igt_power.c', > > 'igt_primes.c', > > + 'igt_pci.c', > > 'igt_rand.c', > > 'igt_stats.c', > > 'igt_syncobj.c', ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v3 1/5] lib/igt_pci: helpers to get PCI capabilities offset 2022-11-30 12:43 ` Kamil Konieczny @ 2022-11-30 12:56 ` Nilawar, Badal 2022-12-07 17:27 ` Bernatowicz, Marcin 2022-12-07 16:58 ` Bernatowicz, Marcin 1 sibling, 1 reply; 8+ messages in thread From: Nilawar, Badal @ 2022-11-30 12:56 UTC (permalink / raw) To: Kamil Konieczny, igt-dev, Anshuman Gupta, Marcin Bernatowicz, rodrigo.vivi, tilak.tangudu, aravind.iddamsetty On 30-11-2022 18:13, Kamil Konieczny wrote: > Hi, > > On 2022-11-29 at 17:30:54 +0530, Nilawar, Badal wrote: >> Hi Anshuman, >> >> On 23-11-2022 14:24, Anshuman Gupta wrote: >>> Added helper functions to search for PCI capabilities >>> with help of libpciaccess library. >>> >>> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com> >>> Co-developed-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com> >>> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com> >>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> >>> --- >>> lib/igt_pci.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >>> lib/igt_pci.h | 32 ++++++++++++++++++++++++++++++++ >>> lib/meson.build | 1 + >>> 3 files changed, 77 insertions(+) >>> create mode 100644 lib/igt_pci.c >>> create mode 100644 lib/igt_pci.h >>> >>> diff --git a/lib/igt_pci.c b/lib/igt_pci.c >>> new file mode 100644 >>> index 0000000000..bc0369341d >>> --- /dev/null >>> +++ b/lib/igt_pci.c >>> @@ -0,0 +1,44 @@ >>> +// SPDX-License-Identifier: MIT >>> +/* >>> + * Copyright © 2022 Intel Corporation >>> + */ >>> + >>> +#include <pciaccess.h> >>> +#include "igt_pci.h" >>> + >>> +/** >>> + * find_pci_cap_offset: >>> + * @dev: pci device >>> + * @cap_id: searched capability id, 0 means any capability >>> + * @start_offset: offset in config space from which we start searching >>> + * >>> + * return: >>> + * -1 on config read error, 0 if capability is not found, >>> + * otherwise offset at which capability with cap_id is found >>> + */ >>> +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id, >>> + int start_offset) >>> +{ >>> + uint8_t offset; >>> + uint16_t cap_header; >>> + int loop = (PCI_CFG_SPACE_SIZE - PCI_TYPE0_1_HEADER_SIZE) >>> + / sizeof(cap_header); >>> + >>> + if (pci_device_cfg_read_u8(dev, &offset, start_offset)) >>> + return -1; >>> + >>> + while (loop--) { >>> + if (offset < PCI_TYPE0_1_HEADER_SIZE) >>> + break; >>> + >>> + if (pci_device_cfg_read_u16(dev, &cap_header, (offset & 0xFC))) >>> + return -1; >>> + >>> + if (!cap_id || cap_id == (cap_header & 0xFF)) >>> + return offset; >>> + >>> + offset = cap_header >> 8; >> For the last capability, next capability address will always be 0. So above >> instead of using while(loop--) we can use while(offset). >> >> Regards, >> Badal > > This way we are guarded for endless looping, btw check for zero > is just at loop enter (first if). Agreed. It is seen that when config space inaccessible it throws value 0xff. So inside loop we should check if offset or capid != 0xff and break the loop otherwise. > >>> + } >>> + > > I would like to see additional check here: > > if (loop <= 0 && offset) > return -1; > > or maybe assert here ? > >>> + return 0; >>> +} >>> diff --git a/lib/igt_pci.h b/lib/igt_pci.h >>> new file mode 100644 >>> index 0000000000..68afd2dacb >>> --- /dev/null >>> +++ b/lib/igt_pci.h >>> @@ -0,0 +1,32 @@ >>> +// SPDX-License-Identifier: MIT >>> +/* >>> + * Copyright © 2022 Intel Corporation >>> + */ >>> + >>> +#ifndef __IGT_PCI_H__ >>> +#define __IGT_PCI_H__ >>> + >>> +#include <stdint.h> >>> +#include <endian.h> >>> + >>> +/* forward declaration */ >>> +struct pci_device; >>> + >>> +#define PCI_TYPE0_1_HEADER_SIZE 0x40 >>> +#define PCI_CAPS_START 0x34 >>> +#define PCI_CFG_SPACE_SIZE 0x100 >>> +#define PCIE_CFG_SPACE_SIZE 0x1000 > ------------ ^ > Remove this, it is unused. > >>> + >>> +enum pci_cap_id { >>> + PCI_EXPRESS_CAP_ID = 0x10 >>> +}; >>> + >>> +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id, >>> + int start_offset); >>> + >>> +static inline int find_pci_cap_offset(struct pci_device *dev, enum pci_cap_id cap_id) > ---- ^ ---- ^ > Remove this, it is work for optimizitaion in compiler, no need > for it here. > Btw do you expect user to use both functions ? > > +cc Marcin > > Regards, > Kamil > >>> +{ >>> + return find_pci_cap_offset_at(dev, cap_id, PCI_CAPS_START); >>> +} >>> + >>> +#endif >>> diff --git a/lib/meson.build b/lib/meson.build >>> index cef2d0ff3d..e0fb7ddfed 100644 >>> --- a/lib/meson.build >>> +++ b/lib/meson.build >>> @@ -33,6 +33,7 @@ lib_sources = [ >>> 'igt_pipe_crc.c', >>> 'igt_power.c', >>> 'igt_primes.c', >>> + 'igt_pci.c', >>> 'igt_rand.c', >>> 'igt_stats.c', >>> 'igt_syncobj.c', ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v3 1/5] lib/igt_pci: helpers to get PCI capabilities offset 2022-11-30 12:56 ` Nilawar, Badal @ 2022-12-07 17:27 ` Bernatowicz, Marcin 0 siblings, 0 replies; 8+ messages in thread From: Bernatowicz, Marcin @ 2022-12-07 17:27 UTC (permalink / raw) To: Nilawar, Badal, Kamil Konieczny, igt-dev, Anshuman Gupta, rodrigo.vivi, tilak.tangudu, aravind.iddamsetty On 11/30/2022 1:56 PM, Nilawar, Badal wrote: > > > On 30-11-2022 18:13, Kamil Konieczny wrote: >> Hi, >> >> On 2022-11-29 at 17:30:54 +0530, Nilawar, Badal wrote: >>> Hi Anshuman, >>> >>> On 23-11-2022 14:24, Anshuman Gupta wrote: >>>> Added helper functions to search for PCI capabilities >>>> with help of libpciaccess library. >>>> >>>> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com> >>>> Co-developed-by: Marcin Bernatowicz >>>> <marcin.bernatowicz@linux.intel.com> >>>> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com> >>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> >>>> --- >>>> lib/igt_pci.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >>>> lib/igt_pci.h | 32 ++++++++++++++++++++++++++++++++ >>>> lib/meson.build | 1 + >>>> 3 files changed, 77 insertions(+) >>>> create mode 100644 lib/igt_pci.c >>>> create mode 100644 lib/igt_pci.h >>>> >>>> diff --git a/lib/igt_pci.c b/lib/igt_pci.c >>>> new file mode 100644 >>>> index 0000000000..bc0369341d >>>> --- /dev/null >>>> +++ b/lib/igt_pci.c >>>> @@ -0,0 +1,44 @@ >>>> +// SPDX-License-Identifier: MIT >>>> +/* >>>> + * Copyright © 2022 Intel Corporation >>>> + */ >>>> + >>>> +#include <pciaccess.h> >>>> +#include "igt_pci.h" >>>> + >>>> +/** >>>> + * find_pci_cap_offset: >>>> + * @dev: pci device >>>> + * @cap_id: searched capability id, 0 means any capability >>>> + * @start_offset: offset in config space from which we start searching >>>> + * >>>> + * return: >>>> + * -1 on config read error, 0 if capability is not found, >>>> + * otherwise offset at which capability with cap_id is found >>>> + */ >>>> +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id >>>> cap_id, >>>> + int start_offset) >>>> +{ >>>> + uint8_t offset; >>>> + uint16_t cap_header; >>>> + int loop = (PCI_CFG_SPACE_SIZE - PCI_TYPE0_1_HEADER_SIZE) >>>> + / sizeof(cap_header); >>>> + >>>> + if (pci_device_cfg_read_u8(dev, &offset, start_offset)) >>>> + return -1; >>>> + >>>> + while (loop--) { >>>> + if (offset < PCI_TYPE0_1_HEADER_SIZE) >>>> + break; >>>> + >>>> + if (pci_device_cfg_read_u16(dev, &cap_header, (offset & >>>> 0xFC))) >>>> + return -1; >>>> + >>>> + if (!cap_id || cap_id == (cap_header & 0xFF)) >>>> + return offset; >>>> + >>>> + offset = cap_header >> 8; >>> For the last capability, next capability address will always be 0. So >>> above >>> instead of using while(loop--) we can use while(offset). >>> >>> Regards, >>> Badal >> >> This way we are guarded for endless looping, btw check for zero >> is just at loop enter (first if). > Agreed. > > It is seen that when config space inaccessible it throws value 0xff. > So inside loop we should check if offset or capid != 0xff and break the > loop otherwise. In such case I would expect pci_device_cfg_read_XXX to return != 0 => break the loop. >> >>>> + } >>>> + >> >> I would like to see additional check here: >> >> if (loop <= 0 && offset) >> return -1; >> >> or maybe assert here ? >> >>>> + return 0; >>>> +} >>>> diff --git a/lib/igt_pci.h b/lib/igt_pci.h >>>> new file mode 100644 >>>> index 0000000000..68afd2dacb >>>> --- /dev/null >>>> +++ b/lib/igt_pci.h >>>> @@ -0,0 +1,32 @@ >>>> +// SPDX-License-Identifier: MIT >>>> +/* >>>> + * Copyright © 2022 Intel Corporation >>>> + */ >>>> + >>>> +#ifndef __IGT_PCI_H__ >>>> +#define __IGT_PCI_H__ >>>> + >>>> +#include <stdint.h> >>>> +#include <endian.h> >>>> + >>>> +/* forward declaration */ >>>> +struct pci_device; >>>> + >>>> +#define PCI_TYPE0_1_HEADER_SIZE 0x40 >>>> +#define PCI_CAPS_START 0x34 >>>> +#define PCI_CFG_SPACE_SIZE 0x100 >>>> +#define PCIE_CFG_SPACE_SIZE 0x1000 >> ------------ ^ >> Remove this, it is unused. >> >>>> + >>>> +enum pci_cap_id { >>>> + PCI_EXPRESS_CAP_ID = 0x10 >>>> +}; >>>> + >>>> +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id >>>> cap_id, >>>> + int start_offset); >>>> + >>>> +static inline int find_pci_cap_offset(struct pci_device *dev, enum >>>> pci_cap_id cap_id) >> ---- ^ ---- ^ >> Remove this, it is work for optimizitaion in compiler, no need >> for it here. >> Btw do you expect user to use both functions ? >> >> +cc Marcin >> >> Regards, >> Kamil >> >>>> +{ >>>> + return find_pci_cap_offset_at(dev, cap_id, PCI_CAPS_START); >>>> +} >>>> + >>>> +#endif >>>> diff --git a/lib/meson.build b/lib/meson.build >>>> index cef2d0ff3d..e0fb7ddfed 100644 >>>> --- a/lib/meson.build >>>> +++ b/lib/meson.build >>>> @@ -33,6 +33,7 @@ lib_sources = [ >>>> 'igt_pipe_crc.c', >>>> 'igt_power.c', >>>> 'igt_primes.c', >>>> + 'igt_pci.c', >>>> 'igt_rand.c', >>>> 'igt_stats.c', >>>> 'igt_syncobj.c', ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v3 1/5] lib/igt_pci: helpers to get PCI capabilities offset 2022-11-30 12:43 ` Kamil Konieczny 2022-11-30 12:56 ` Nilawar, Badal @ 2022-12-07 16:58 ` Bernatowicz, Marcin 1 sibling, 0 replies; 8+ messages in thread From: Bernatowicz, Marcin @ 2022-12-07 16:58 UTC (permalink / raw) To: Kamil Konieczny, igt-dev, Badal Nilawar, Anshuman Gupta, rodrigo.vivi, tilak.tangudu, aravind.iddamsetty On 11/30/2022 1:43 PM, Kamil Konieczny wrote: > Hi, > > On 2022-11-29 at 17:30:54 +0530, Nilawar, Badal wrote: >> Hi Anshuman, >> >> On 23-11-2022 14:24, Anshuman Gupta wrote: >>> Added helper functions to search for PCI capabilities >>> with help of libpciaccess library. >>> >>> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com> >>> Co-developed-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com> >>> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com> >>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> >>> --- >>> lib/igt_pci.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >>> lib/igt_pci.h | 32 ++++++++++++++++++++++++++++++++ >>> lib/meson.build | 1 + >>> 3 files changed, 77 insertions(+) >>> create mode 100644 lib/igt_pci.c >>> create mode 100644 lib/igt_pci.h >>> >>> diff --git a/lib/igt_pci.c b/lib/igt_pci.c >>> new file mode 100644 >>> index 0000000000..bc0369341d >>> --- /dev/null >>> +++ b/lib/igt_pci.c >>> @@ -0,0 +1,44 @@ >>> +// SPDX-License-Identifier: MIT >>> +/* >>> + * Copyright © 2022 Intel Corporation >>> + */ >>> + >>> +#include <pciaccess.h> >>> +#include "igt_pci.h" >>> + >>> +/** >>> + * find_pci_cap_offset: >>> + * @dev: pci device >>> + * @cap_id: searched capability id, 0 means any capability >>> + * @start_offset: offset in config space from which we start searching >>> + * >>> + * return: >>> + * -1 on config read error, 0 if capability is not found, >>> + * otherwise offset at which capability with cap_id is found >>> + */ >>> +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id, >>> + int start_offset) >>> +{ >>> + uint8_t offset; >>> + uint16_t cap_header; >>> + int loop = (PCI_CFG_SPACE_SIZE - PCI_TYPE0_1_HEADER_SIZE) >>> + / sizeof(cap_header); >>> + >>> + if (pci_device_cfg_read_u8(dev, &offset, start_offset)) >>> + return -1; >>> + >>> + while (loop--) { >>> + if (offset < PCI_TYPE0_1_HEADER_SIZE) >>> + break; >>> + >>> + if (pci_device_cfg_read_u16(dev, &cap_header, (offset & 0xFC))) >>> + return -1; >>> + >>> + if (!cap_id || cap_id == (cap_header & 0xFF)) >>> + return offset; >>> + >>> + offset = cap_header >> 8; >> For the last capability, next capability address will always be 0. So above >> instead of using while(loop--) we can use while(offset). >> >> Regards, >> Badal > > This way we are guarded for endless looping, btw check for zero > is just at loop enter (first if). > >>> + } >>> + > > I would like to see additional check here: > > if (loop <= 0 && offset) > return -1; > > or maybe assert here ? why ? the -1 is on config read error * return: * -1 on config read error, 0 if capability is not found, * otherwise offset at which capability with cap_id is found > >>> + return 0; >>> +} >>> diff --git a/lib/igt_pci.h b/lib/igt_pci.h >>> new file mode 100644 >>> index 0000000000..68afd2dacb >>> --- /dev/null >>> +++ b/lib/igt_pci.h >>> @@ -0,0 +1,32 @@ >>> +// SPDX-License-Identifier: MIT >>> +/* >>> + * Copyright © 2022 Intel Corporation >>> + */ >>> + >>> +#ifndef __IGT_PCI_H__ >>> +#define __IGT_PCI_H__ >>> + >>> +#include <stdint.h> >>> +#include <endian.h> >>> + >>> +/* forward declaration */ >>> +struct pci_device; >>> + >>> +#define PCI_TYPE0_1_HEADER_SIZE 0x40 >>> +#define PCI_CAPS_START 0x34 >>> +#define PCI_CFG_SPACE_SIZE 0x100 >>> +#define PCIE_CFG_SPACE_SIZE 0x1000 > ------------ ^ > Remove this, it is unused. > >>> + >>> +enum pci_cap_id { >>> + PCI_EXPRESS_CAP_ID = 0x10 >>> +}; >>> + >>> +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id, >>> + int start_offset); >>> + >>> +static inline int find_pci_cap_offset(struct pci_device *dev, enum pci_cap_id cap_id) > ---- ^ ---- ^ > Remove this, it is work for optimizitaion in compiler, no need > for it here. > Btw do you expect user to use both functions ? > > +cc Marcin > > Regards, > Kamil > >>> +{ >>> + return find_pci_cap_offset_at(dev, cap_id, PCI_CAPS_START); >>> +} >>> + >>> +#endif >>> diff --git a/lib/meson.build b/lib/meson.build >>> index cef2d0ff3d..e0fb7ddfed 100644 >>> --- a/lib/meson.build >>> +++ b/lib/meson.build >>> @@ -33,6 +33,7 @@ lib_sources = [ >>> 'igt_pipe_crc.c', >>> 'igt_power.c', >>> 'igt_primes.c', >>> + 'igt_pci.c', >>> 'igt_rand.c', >>> 'igt_stats.c', >>> 'igt_syncobj.c', ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-12-07 17:28 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-11-23 8:52 [igt-dev] [PATCH i-g-t v3 0/5] Cold Reset IGT Test Anshuman Gupta 2022-11-23 8:52 ` [igt-dev] [PATCH i-g-t v3 1/5] lib/igt_pci: helpers to get PCI capabilities offset Anshuman Gupta -- strict thread matches above, loose matches on Subject: below -- 2022-11-23 8:54 [igt-dev] [PATCH i-g-t v3 0/5] Cold Reset IGT Test Anshuman Gupta 2022-11-23 8:54 ` [igt-dev] [PATCH i-g-t v3 1/5] lib/igt_pci: helpers to get PCI capabilities offset Anshuman Gupta 2022-11-29 12:00 ` Nilawar, Badal 2022-11-30 12:43 ` Kamil Konieczny 2022-11-30 12:56 ` Nilawar, Badal 2022-12-07 17:27 ` Bernatowicz, Marcin 2022-12-07 16:58 ` Bernatowicz, Marcin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox