From: Bjorn Helgaas <bhelgaas@google.com>
To: Wei Yang <weiyang@linux.vnet.ibm.com>
Cc: linux-pci@vger.kernel.org, Yinghai Lu <yinghai@kernel.org>
Subject: Re: [PATCH 2/3] PCI: use u8 instead of int for pci configuration space pos and cap
Date: Wed, 22 Oct 2014 17:23:07 -0600 [thread overview]
Message-ID: <20141022232307.GD4795@google.com> (raw)
In-Reply-To: <1413269253-8990-3-git-send-email-weiyang@linux.vnet.ibm.com>
[+cc Yinghai]
On Tue, Oct 14, 2014 at 02:47:31PM +0800, Wei Yang wrote:
> For pci devices complying with PCI LB 3.0, the configuration space size is
> 256 and the Cap ID is represented by a 8bit field. This means a type of u8 is
> enough to repsent the Cap's position and ID.
>
> This patch does some clean up for the Cap position and ID by replacing the int
> type with u8. And consolidate the check of whether a Cap is represented from
> (pos <= 0) to (!pos).
>
> And two functions use char to represent cap, it is changed to u8 in this
> patch.
Please spell-check the changelog. Please also split the return value check
changes ("pos <= 0" to "!pos") into a separate patch. That part is not
obviously correct.
>
> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> ---
> drivers/pci/pci.c | 55 +++++++++++++++++++++++++-------------------------
> drivers/pci/probe.c | 8 ++++----
> drivers/pci/quirks.c | 19 ++++++++---------
> include/linux/pci.h | 20 +++++++++---------
> 4 files changed, 52 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 76b002b1..47d8d0c 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -138,8 +138,8 @@ EXPORT_SYMBOL_GPL(pci_ioremap_bar);
> #endif
>
>
> -static int __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int devfn,
> - u8 pos, int cap, int *ttl)
> +static u8 __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int devfn,
> + u8 pos, u8 cap, int *ttl)
> {
> u8 id;
>
> @@ -159,22 +159,22 @@ static int __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int devfn,
> return 0;
> }
>
> -static int __pci_find_next_cap(struct pci_bus *bus, unsigned int devfn,
> - u8 pos, int cap)
> +static u8 __pci_find_next_cap(struct pci_bus *bus, unsigned int devfn,
> + u8 pos, u8 cap)
> {
> int ttl = PCI_FIND_CAP_TTL;
>
> return __pci_find_next_cap_ttl(bus, devfn, pos, cap, &ttl);
> }
>
> -int pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap)
> +u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, u8 cap)
> {
> return __pci_find_next_cap(dev->bus, dev->devfn,
> pos + PCI_CAP_LIST_NEXT, cap);
> }
> EXPORT_SYMBOL_GPL(pci_find_next_capability);
>
> -static int __pci_bus_find_cap_start(struct pci_bus *bus,
> +static u8 __pci_bus_find_cap_start(struct pci_bus *bus,
> unsigned int devfn, u8 hdr_type)
> {
> u16 status;
> @@ -215,9 +215,9 @@ static int __pci_bus_find_cap_start(struct pci_bus *bus,
> * %PCI_CAP_ID_PCIX PCI-X
> * %PCI_CAP_ID_EXP PCI Express
> */
> -int pci_find_capability(struct pci_dev *dev, int cap)
> +u8 pci_find_capability(struct pci_dev *dev, u8 cap)
> {
> - int pos;
> + u8 pos;
>
> pos = __pci_bus_find_cap_start(dev->bus, dev->devfn, dev->hdr_type);
> if (pos)
> @@ -240,9 +240,9 @@ EXPORT_SYMBOL(pci_find_capability);
> * device's PCI configuration space or 0 in case the device does not
> * support it.
> */
> -int pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap)
> +u8 pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, u8 cap)
> {
> - int pos;
> + u8 pos;
> u8 hdr_type;
>
> pci_bus_read_config_byte(bus, devfn, PCI_HEADER_TYPE, &hdr_type);
> @@ -327,7 +327,7 @@ int pci_find_ext_capability(struct pci_dev *dev, int cap)
> }
> EXPORT_SYMBOL_GPL(pci_find_ext_capability);
>
> -static int __pci_find_next_ht_cap(struct pci_dev *dev, int pos, int ht_cap)
> +static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
> {
> int rc, ttl = PCI_FIND_CAP_TTL;
> u8 cap, mask;
> @@ -367,7 +367,7 @@ static int __pci_find_next_ht_cap(struct pci_dev *dev, int pos, int ht_cap)
> * NB. To be 100% safe against broken PCI devices, the caller should take
> * steps to avoid an infinite loop.
> */
> -int pci_find_next_ht_capability(struct pci_dev *dev, int pos, int ht_cap)
> +u8 pci_find_next_ht_capability(struct pci_dev *dev, u8 pos, int ht_cap)
> {
> return __pci_find_next_ht_cap(dev, pos + PCI_CAP_LIST_NEXT, ht_cap);
> }
> @@ -384,9 +384,9 @@ EXPORT_SYMBOL_GPL(pci_find_next_ht_capability);
> * The address points to the PCI capability, of type PCI_CAP_ID_HT,
> * which has a Hypertransport capability matching @ht_cap.
> */
> -int pci_find_ht_capability(struct pci_dev *dev, int ht_cap)
> +u8 pci_find_ht_capability(struct pci_dev *dev, int ht_cap)
> {
> - int pos;
> + u8 pos;
>
> pos = __pci_bus_find_cap_start(dev->bus, dev->devfn, dev->hdr_type);
> if (pos)
> @@ -896,7 +896,7 @@ static struct pci_cap_saved_state *_pci_find_saved_cap(struct pci_dev *pci_dev,
> return NULL;
> }
>
> -struct pci_cap_saved_state *pci_find_saved_cap(struct pci_dev *dev, char cap)
> +struct pci_cap_saved_state *pci_find_saved_cap(struct pci_dev *dev, u8 cap)
> {
> return _pci_find_saved_cap(dev, cap, false);
> }
> @@ -956,11 +956,11 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
>
> static int pci_save_pcix_state(struct pci_dev *dev)
> {
> - int pos;
> + u8 pos;
> struct pci_cap_saved_state *save_state;
>
> pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
> - if (pos <= 0)
> + if (!pos)
> return 0;
>
> save_state = pci_find_saved_cap(dev, PCI_CAP_ID_PCIX);
> @@ -977,13 +977,14 @@ static int pci_save_pcix_state(struct pci_dev *dev)
>
> static void pci_restore_pcix_state(struct pci_dev *dev)
> {
> - int i = 0, pos;
> + int i = 0;
> + u8 pos;
> struct pci_cap_saved_state *save_state;
> u16 *cap;
>
> save_state = pci_find_saved_cap(dev, PCI_CAP_ID_PCIX);
> pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
> - if (!save_state || pos <= 0)
> + if (!save_state || !pos)
> return;
> cap = (u16 *)&save_state->cap.data[0];
>
> @@ -2036,7 +2037,7 @@ void pci_config_pm_runtime_put(struct pci_dev *pdev)
> */
> void pci_pm_init(struct pci_dev *dev)
> {
> - int pm;
> + u8 pm;
> u16 pmc;
>
> pm_runtime_forbid(&dev->dev);
> @@ -2126,7 +2127,7 @@ static int _pci_add_cap_save_buffer(struct pci_dev *dev, u16 cap,
> else
> pos = pci_find_capability(dev, cap);
>
> - if (pos <= 0)
> + if (!pos)
> return 0;
>
> save_state = kzalloc(sizeof(*save_state) + size, GFP_KERNEL);
> @@ -2141,7 +2142,7 @@ static int _pci_add_cap_save_buffer(struct pci_dev *dev, u16 cap,
> return 0;
> }
>
> -int pci_add_cap_save_buffer(struct pci_dev *dev, char cap, unsigned int size)
> +int pci_add_cap_save_buffer(struct pci_dev *dev, u8 cap, unsigned int size)
> {
> return _pci_add_cap_save_buffer(dev, cap, false, size);
> }
> @@ -3046,7 +3047,7 @@ EXPORT_SYMBOL_GPL(pci_check_and_unmask_intx);
> */
> void pci_msi_off(struct pci_dev *dev)
> {
> - int pos;
> + u8 pos;
> u16 control;
>
> /*
> @@ -3120,7 +3121,7 @@ static int pcie_flr(struct pci_dev *dev, int probe)
>
> static int pci_af_flr(struct pci_dev *dev, int probe)
> {
> - int pos;
> + u8 pos;
> u8 cap;
>
> pos = pci_find_capability(dev, PCI_CAP_ID_AF);
> @@ -3888,7 +3889,7 @@ EXPORT_SYMBOL_GPL(pci_try_reset_bus);
> */
> int pcix_get_max_mmrbc(struct pci_dev *dev)
> {
> - int cap;
> + u8 cap;
> u32 stat;
>
> cap = pci_find_capability(dev, PCI_CAP_ID_PCIX);
> @@ -3911,7 +3912,7 @@ EXPORT_SYMBOL(pcix_get_max_mmrbc);
> */
> int pcix_get_mmrbc(struct pci_dev *dev)
> {
> - int cap;
> + u8 cap;
> u16 cmd;
>
> cap = pci_find_capability(dev, PCI_CAP_ID_PCIX);
> @@ -3936,7 +3937,7 @@ EXPORT_SYMBOL(pcix_get_mmrbc);
> */
> int pcix_set_mmrbc(struct pci_dev *dev, int mmrbc)
> {
> - int cap;
> + u8 cap;
> u32 stat, v, o;
> u16 cmd;
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 4170113..9b316bf 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -606,7 +606,7 @@ static enum pci_bus_speed agp_speed(int agp3, int agpstat)
> static void pci_set_bus_speed(struct pci_bus *bus)
> {
> struct pci_dev *bridge = bus->self;
> - int pos;
> + u8 pos;
>
> pos = pci_find_capability(bridge, PCI_CAP_ID_AGP);
> if (!pos)
> @@ -958,7 +958,7 @@ static void pci_read_irq(struct pci_dev *dev)
>
> void set_pcie_port_type(struct pci_dev *pdev)
> {
> - int pos;
> + u8 pos;
> u16 reg16;
>
> pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
> @@ -1046,7 +1046,7 @@ static int pci_cfg_space_size_ext(struct pci_dev *dev)
>
> int pci_cfg_space_size(struct pci_dev *dev)
> {
> - int pos;
> + u8 pos;
> u32 status;
> u16 class;
>
> @@ -1087,7 +1087,7 @@ int pci_setup_device(struct pci_dev *dev)
> u32 class;
> u8 hdr_type;
> struct pci_slot *slot;
> - int pos = 0;
> + u8 pos = 0;
> struct pci_bus_region region;
> struct resource *res;
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a5f46b8..15b76a0 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -2176,7 +2176,7 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x9601, quirk_amd_780_apc_msi);
> * return 1 if a HT MSI capability is found and enabled */
> static int msi_ht_cap_enabled(struct pci_dev *dev)
> {
> - int pos, ttl = PCI_FIND_CAP_TTL;
> + u8 pos, ttl = PCI_FIND_CAP_TTL;
>
> pos = pci_find_ht_capability(dev, HT_CAPTYPE_MSI_MAPPING);
> while (pos && ttl--) {
> @@ -2235,7 +2235,7 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_DEVICE_ID_NVIDIA_CK804_PCIE,
> /* Force enable MSI mapping capability on HT bridges */
> static void ht_enable_msi_mapping(struct pci_dev *dev)
> {
> - int pos, ttl = PCI_FIND_CAP_TTL;
> + u8 pos, ttl = PCI_FIND_CAP_TTL;
>
> pos = pci_find_ht_capability(dev, HT_CAPTYPE_MSI_MAPPING);
> while (pos && ttl--) {
> @@ -2314,7 +2314,7 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA,
>
> static int ht_check_msi_mapping(struct pci_dev *dev)
> {
> - int pos, ttl = PCI_FIND_CAP_TTL;
> + u8 pos, ttl = PCI_FIND_CAP_TTL;
> int found = 0;
>
> /* check if there is HT MSI cap or enabled on this device */
> @@ -2343,7 +2343,7 @@ static int ht_check_msi_mapping(struct pci_dev *dev)
> static int host_bridge_with_leaf(struct pci_dev *host_bridge)
> {
> struct pci_dev *dev;
> - int pos;
> + u8 pos;
> int i, dev_no;
> int found = 0;
>
> @@ -2376,7 +2376,8 @@ static int host_bridge_with_leaf(struct pci_dev *host_bridge)
>
> static int is_end_of_ht_chain(struct pci_dev *dev)
> {
> - int pos, ctrl_off;
> + int ctrl_off;
> + u8 pos;
> int end = 0;
> u16 flags, ctrl;
>
> @@ -2401,7 +2402,7 @@ out:
> static void nv_ht_enable_msi_mapping(struct pci_dev *dev)
> {
> struct pci_dev *host_bridge;
> - int pos;
> + u8 pos;
> int i, dev_no;
> int found = 0;
>
> @@ -2439,7 +2440,7 @@ out:
>
> static void ht_disable_msi_mapping(struct pci_dev *dev)
> {
> - int pos, ttl = PCI_FIND_CAP_TTL;
> + u8 pos, ttl = PCI_FIND_CAP_TTL;
>
> pos = pci_find_ht_capability(dev, HT_CAPTYPE_MSI_MAPPING);
> while (pos && ttl--) {
> @@ -2460,7 +2461,7 @@ static void ht_disable_msi_mapping(struct pci_dev *dev)
> static void __nv_msi_ht_cap_quirk(struct pci_dev *dev, int all)
> {
> struct pci_dev *host_bridge;
> - int pos;
> + u8 pos;
> int found;
>
> if (!pci_msi_enabled())
> @@ -3226,7 +3227,7 @@ fs_initcall_sync(pci_apply_final_quirks);
> */
> static int reset_intel_generic_dev(struct pci_dev *dev, int probe)
> {
> - int pos;
> + u8 pos;
>
> /* only implement PCI_CLASS_SERIAL_USB at present */
> if (dev->class == PCI_CLASS_SERIAL_USB) {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index b27b79e..399a18a 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -818,12 +818,12 @@ enum pci_lost_interrupt_reason {
> PCI_LOST_IRQ_DISABLE_ACPI,
> };
> enum pci_lost_interrupt_reason pci_lost_interrupt(struct pci_dev *dev);
> -int pci_find_capability(struct pci_dev *dev, int cap);
> -int pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap);
> +u8 pci_find_capability(struct pci_dev *dev, u8 cap);
> +u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, u8 cap);
> int pci_find_ext_capability(struct pci_dev *dev, int cap);
> int pci_find_next_ext_capability(struct pci_dev *dev, int pos, int cap);
> -int pci_find_ht_capability(struct pci_dev *dev, int ht_cap);
> -int pci_find_next_ht_capability(struct pci_dev *dev, int pos, int ht_cap);
> +u8 pci_find_ht_capability(struct pci_dev *dev, int ht_cap);
> +u8 pci_find_next_ht_capability(struct pci_dev *dev, u8 pos, int ht_cap);
> struct pci_bus *pci_find_next_bus(const struct pci_bus *from);
>
> struct pci_dev *pci_get_device(unsigned int vendor, unsigned int device,
> @@ -1004,10 +1004,10 @@ void pci_restore_state(struct pci_dev *dev);
> struct pci_saved_state *pci_store_saved_state(struct pci_dev *dev);
> int pci_load_and_free_saved_state(struct pci_dev *dev,
> struct pci_saved_state **state);
> -struct pci_cap_saved_state *pci_find_saved_cap(struct pci_dev *dev, char cap);
> +struct pci_cap_saved_state *pci_find_saved_cap(struct pci_dev *dev, u8 cap);
> struct pci_cap_saved_state *pci_find_saved_ext_cap(struct pci_dev *dev,
> u16 cap);
> -int pci_add_cap_save_buffer(struct pci_dev *dev, char cap, unsigned int size);
> +int pci_add_cap_save_buffer(struct pci_dev *dev, u8 cap, unsigned int size);
> int pci_add_ext_cap_save_buffer(struct pci_dev *dev,
> u16 cap, unsigned int size);
> int __pci_complete_power_transition(struct pci_dev *dev, pci_power_t state);
> @@ -1045,7 +1045,7 @@ void set_pcie_port_type(struct pci_dev *pdev);
> void set_pcie_hotplug_bridge(struct pci_dev *pdev);
>
> /* Functions for PCI Hotplug drivers to use */
> -int pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap);
> +u8 pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, u8 cap);
> unsigned int pci_rescan_bus_bridge_resize(struct pci_dev *bridge);
> unsigned int pci_rescan_bus(struct pci_bus *bus);
> void pci_lock_rescan_remove(void);
> @@ -1360,10 +1360,10 @@ static inline int __pci_register_driver(struct pci_driver *drv,
> static inline int pci_register_driver(struct pci_driver *drv)
> { return 0; }
> static inline void pci_unregister_driver(struct pci_driver *drv) { }
> -static inline int pci_find_capability(struct pci_dev *dev, int cap)
> +static inline u8 pci_find_capability(struct pci_dev *dev, u8 cap)
> { return 0; }
> -static inline int pci_find_next_capability(struct pci_dev *dev, u8 post,
> - int cap)
> +static inline u8 pci_find_next_capability(struct pci_dev *dev, u8 post,
> + u8 cap)
> { return 0; }
> static inline int pci_find_ext_capability(struct pci_dev *dev, int cap)
> { return 0; }
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2014-10-22 23:23 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-10-14 6:47 [PATCH 0/3] PCI: code clean up on pci configuration space Wei Yang
2014-10-14 6:47 ` [PATCH 1/3] PCI: move PCI_FIND_CAP_TTL to pci.h and use it in quirks Wei Yang
2014-10-22 23:18 ` Bjorn Helgaas
2014-10-24 3:44 ` Wei Yang
2014-11-06 3:12 ` Wei Yang
2014-11-18 9:16 ` Wei Yang
2014-10-14 6:47 ` [PATCH 2/3] PCI: use u8 instead of int for pci configuration space pos and cap Wei Yang
2014-10-22 23:23 ` Bjorn Helgaas [this message]
2014-10-24 3:49 ` Wei Yang
2014-10-14 6:47 ` [PATCH 3/3] PCI: use u16 instead of int for pci express extended capabilities " Wei Yang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20141022232307.GD4795@google.com \
--to=bhelgaas@google.com \
--cc=linux-pci@vger.kernel.org \
--cc=weiyang@linux.vnet.ibm.com \
--cc=yinghai@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).