Patchwork pci: aspm: make pcie_capability a packed struct type

login
register
mail settings
Submitter Colin King
Date Dec. 7, 2012, 12:02 a.m.
Message ID <1354838564-6033-1-git-send-email-colin.king@canonical.com>
Download mbox | patch
Permalink /patch/204349/
State Accepted
Headers show

Comments

Colin King - Dec. 7, 2012, 12:02 a.m.
From: Colin Ian King <colin.king@canonical.com>

Since we are mapping the pcie_capabiliy struct 1-to-1 onto real
data we should give it a packed attribute to ensure GCC does not
stuff in extra padding.

I took the liberty of making this a typedef to make it a little
simpler than having packed structs everywhere.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/pci/aspm/aspm.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
Keng-Yu Lin - Dec. 10, 2012, 7:20 a.m.
On Fri, Dec 7, 2012 at 8:02 AM, Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Since we are mapping the pcie_capabiliy struct 1-to-1 onto real
> data we should give it a packed attribute to ensure GCC does not
> stuff in extra padding.
>
> I took the liberty of making this a typedef to make it a little
> simpler than having packed structs everywhere.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/pci/aspm/aspm.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/src/pci/aspm/aspm.c b/src/pci/aspm/aspm.c
> index e6d91ca..1a6cc27 100644
> --- a/src/pci/aspm/aspm.c
> +++ b/src/pci/aspm/aspm.c
> @@ -61,7 +61,7 @@ struct pci_device {
>  /* PCI Express Capability Structure is defined in Section 7.8
>   * of PCI Express®i Base Specification Revision 2.0
>   */
> -struct pcie_capability {
> +typedef struct {
>         uint8_t pcie_cap_id;
>         uint8_t next_cap_point;
>         uint16_t pcie_cap_reg;
> @@ -86,7 +86,7 @@ struct pcie_capability {
>         uint32_t slot_cap2;
>         uint16_t slot_contrl2;
>         uint16_t slot_status2;
> -};
> +} __attribute__ ((packed)) pcie_capability;
>
>  static int facp_get_aspm_control(fwts_framework *fw, int *aspm)
>  {
> @@ -118,28 +118,28 @@ static int pcie_compare_rp_dev_aspm_registers(fwts_framework *fw,
>         struct pci_device *rp,
>         struct pci_device *dev)
>  {
> -       struct pcie_capability *rp_cap, *device_cap;
> +       pcie_capability *rp_cap, *device_cap;
>         uint8_t rp_aspm_cntrl, device_aspm_cntrl;
>         uint8_t next_cap;
>         int ret = FWTS_OK;
>         bool l0s_disabled = false, l1_disabled = false;
>
>         next_cap = rp->config[FWTS_PCI_CAPABILITIES_POINTER];
> -       rp_cap = (struct pcie_capability *) &rp->config[next_cap];
> +       rp_cap = (pcie_capability *) &rp->config[next_cap];
>         while (rp_cap->pcie_cap_id != FWTS_PCI_EXPRESS_CAP_ID) {
>                 if (rp_cap->next_cap_point == FWTS_PCI_LAST_ID)
>                         break;
>                 next_cap = rp_cap->next_cap_point;
> -               rp_cap = (struct pcie_capability *) &rp->config[next_cap];
> +               rp_cap = (pcie_capability *) &rp->config[next_cap];
>         }
>
>         next_cap = dev->config[FWTS_PCI_CAPABILITIES_POINTER];
> -       device_cap = (struct pcie_capability *) &dev->config[next_cap];
> +       device_cap = (pcie_capability *)&dev->config[next_cap];
>         while (device_cap->pcie_cap_id != FWTS_PCI_EXPRESS_CAP_ID) {
>                 if (device_cap->next_cap_point == FWTS_PCI_LAST_ID)
>                         break;
>                 next_cap = device_cap->next_cap_point;
> -               device_cap = (struct pcie_capability *) &dev->config[next_cap];
> +               device_cap = (pcie_capability *)&dev->config[next_cap];
>         }
>
>
> --
> 1.8.0
>
Acked-by: Keng-Yu Lin <kengyu@canonical.com>
Ivan Hu - Dec. 13, 2012, 9:42 a.m.
On 12/07/2012 08:02 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Since we are mapping the pcie_capabiliy struct 1-to-1 onto real
> data we should give it a packed attribute to ensure GCC does not
> stuff in extra padding.
>
> I took the liberty of making this a typedef to make it a little
> simpler than having packed structs everywhere.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/pci/aspm/aspm.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/src/pci/aspm/aspm.c b/src/pci/aspm/aspm.c
> index e6d91ca..1a6cc27 100644
> --- a/src/pci/aspm/aspm.c
> +++ b/src/pci/aspm/aspm.c
> @@ -61,7 +61,7 @@ struct pci_device {
>   /* PCI Express Capability Structure is defined in Section 7.8
>    * of PCI Express®i Base Specification Revision 2.0
>    */
> -struct pcie_capability {
> +typedef struct {
>   	uint8_t pcie_cap_id;
>   	uint8_t next_cap_point;
>   	uint16_t pcie_cap_reg;
> @@ -86,7 +86,7 @@ struct pcie_capability {
>   	uint32_t slot_cap2;
>   	uint16_t slot_contrl2;
>   	uint16_t slot_status2;
> -};
> +} __attribute__ ((packed)) pcie_capability;
>
>   static int facp_get_aspm_control(fwts_framework *fw, int *aspm)
>   {
> @@ -118,28 +118,28 @@ static int pcie_compare_rp_dev_aspm_registers(fwts_framework *fw,
>   	struct pci_device *rp,
>   	struct pci_device *dev)
>   {
> -	struct pcie_capability *rp_cap, *device_cap;
> +	pcie_capability *rp_cap, *device_cap;
>   	uint8_t rp_aspm_cntrl, device_aspm_cntrl;
>   	uint8_t next_cap;
>   	int ret = FWTS_OK;
>   	bool l0s_disabled = false, l1_disabled = false;
>
>   	next_cap = rp->config[FWTS_PCI_CAPABILITIES_POINTER];
> -	rp_cap = (struct pcie_capability *) &rp->config[next_cap];
> +	rp_cap = (pcie_capability *) &rp->config[next_cap];
>   	while (rp_cap->pcie_cap_id != FWTS_PCI_EXPRESS_CAP_ID) {
>   		if (rp_cap->next_cap_point == FWTS_PCI_LAST_ID)
>   			break;
>   		next_cap = rp_cap->next_cap_point;
> -		rp_cap = (struct pcie_capability *) &rp->config[next_cap];
> +		rp_cap = (pcie_capability *) &rp->config[next_cap];
>   	}
>
>   	next_cap = dev->config[FWTS_PCI_CAPABILITIES_POINTER];
> -	device_cap = (struct pcie_capability *)	&dev->config[next_cap];
> +	device_cap = (pcie_capability *)&dev->config[next_cap];
>   	while (device_cap->pcie_cap_id != FWTS_PCI_EXPRESS_CAP_ID) {
>   		if (device_cap->next_cap_point == FWTS_PCI_LAST_ID)
>   			break;
>   		next_cap = device_cap->next_cap_point;
> -		device_cap = (struct pcie_capability *)	&dev->config[next_cap];
> +		device_cap = (pcie_capability *)&dev->config[next_cap];
>   	}
>
>
>
Acked-by: Ivan Hu <ivan.hu@canonical.com>

Patch

diff --git a/src/pci/aspm/aspm.c b/src/pci/aspm/aspm.c
index e6d91ca..1a6cc27 100644
--- a/src/pci/aspm/aspm.c
+++ b/src/pci/aspm/aspm.c
@@ -61,7 +61,7 @@  struct pci_device {
 /* PCI Express Capability Structure is defined in Section 7.8
  * of PCI Express®i Base Specification Revision 2.0
  */
-struct pcie_capability {
+typedef struct {
 	uint8_t pcie_cap_id;
 	uint8_t next_cap_point;
 	uint16_t pcie_cap_reg;
@@ -86,7 +86,7 @@  struct pcie_capability {
 	uint32_t slot_cap2;
 	uint16_t slot_contrl2;
 	uint16_t slot_status2;
-};
+} __attribute__ ((packed)) pcie_capability;
 
 static int facp_get_aspm_control(fwts_framework *fw, int *aspm)
 {
@@ -118,28 +118,28 @@  static int pcie_compare_rp_dev_aspm_registers(fwts_framework *fw,
 	struct pci_device *rp,
 	struct pci_device *dev)
 {
-	struct pcie_capability *rp_cap, *device_cap;
+	pcie_capability *rp_cap, *device_cap;
 	uint8_t rp_aspm_cntrl, device_aspm_cntrl;
 	uint8_t next_cap;
 	int ret = FWTS_OK;
 	bool l0s_disabled = false, l1_disabled = false;
 
 	next_cap = rp->config[FWTS_PCI_CAPABILITIES_POINTER];
-	rp_cap = (struct pcie_capability *) &rp->config[next_cap];
+	rp_cap = (pcie_capability *) &rp->config[next_cap];
 	while (rp_cap->pcie_cap_id != FWTS_PCI_EXPRESS_CAP_ID) {
 		if (rp_cap->next_cap_point == FWTS_PCI_LAST_ID)
 			break;
 		next_cap = rp_cap->next_cap_point;
-		rp_cap = (struct pcie_capability *) &rp->config[next_cap];
+		rp_cap = (pcie_capability *) &rp->config[next_cap];
 	}
 
 	next_cap = dev->config[FWTS_PCI_CAPABILITIES_POINTER];
-	device_cap = (struct pcie_capability *)	&dev->config[next_cap];
+	device_cap = (pcie_capability *)&dev->config[next_cap];
 	while (device_cap->pcie_cap_id != FWTS_PCI_EXPRESS_CAP_ID) {
 		if (device_cap->next_cap_point == FWTS_PCI_LAST_ID)
 			break;
 		next_cap = device_cap->next_cap_point;
-		device_cap = (struct pcie_capability *)	&dev->config[next_cap];
+		device_cap = (pcie_capability *)&dev->config[next_cap];
 	}