netdev - Re: [RFC 2/3] tg3: Use pci_set_power

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1393287337.9208.80.camel@LTIRV-MCHAN1.corp.ad.broadcom.com>
Date:	Mon, 24 Feb 2014 16:15:37 -0800
From:	Michael Chan <mchan@...adcom.com>
To:	Jon Mason <jdmason@...zu.us>
CC:	<netdev@...r.kernel.org>, Nithin Nayak Sujir <nsujir@...adcom.com>
Subject: Re: [RFC 2/3] tg3: Use pci_set_power_state() for setting PM states

On Mon, 2014-02-24 at 15:34 -0700, Jon Mason wrote: 
> Use the existing infrastructure of pci_set_power_state() instead of
> setting the relevant bits via PCI config read/write in the driver.
> Also, use pci_pme_active() to set the PCI_PM_CTRL_PME_ENABLE bit in PCI
> PM control register.  pci_set_power_state() and pci_pme_active() perform
> the same operations as the driver did before, so there should be no
> functional change.  That being said, this has not been tested.
> 
> Signed-off-by: Jon Mason <jdmason@...zu.us>
> Cc: Nithin Nayak Sujir <nsujir@...adcom.com>
> Cc: Michael Chan <mchan@...adcom.com>

I've reviewed the code and this patch should be ok.  The original intent
of the code was to force the power state to D0 in case it was clobbered
by an MMIO write not intended for that register due to a hardware bug.
If it was clobbered like that, pci_set_power_state() may not set it to
D0 if it thinks that it is already in D0.  There is no MMIO write up
until this point so it is ok to call pci_set_power_state().

Acked-by: Michael Chan <mchan@...adcom.com>

> ---
>  drivers/net/ethernet/broadcom/tg3.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index 3167ed6..36c3fd9 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -16362,22 +16362,14 @@ static int tg3_get_invariants(struct tg3 *tp, const struct pci_device_id *ent)
>  		 * for all chip writes not to mailbox registers.
>  		 */
>  		if (tg3_flag(tp, PCIX_MODE)) {
> -			u32 pm_reg;
> -
>  			tg3_flag_set(tp, PCIX_TARGET_HWBUG);
>  
>  			/* The chip can have it's power management PCI config
>  			 * space registers clobbered due to this bug.
>  			 * So explicitly force the chip into D0 here.
>  			 */
> -			pci_read_config_dword(tp->pdev,
> -					      tp->pdev->pm_cap + PCI_PM_CTRL,
> -					      &pm_reg);
> -			pm_reg &= ~PCI_PM_CTRL_STATE_MASK;
> -			pm_reg |= PCI_PM_CTRL_PME_ENABLE | 0 /* D0 */;
> -			pci_write_config_dword(tp->pdev,
> -					       tp->pdev->pm_cap + PCI_PM_CTRL,
> -					       pm_reg);
> +			pci_set_power_state(tp->pdev, PCI_D0);
> +			pci_pme_active(tp->pdev, true);
>  
>  			/* Also, force SERR#/PERR# in PCI command. */
>  			pci_read_config_word(tp->pdev, PCI_COMMAND, &pci_cmd);


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html