Re: [PATCH 4/7] em: Improve access logic for software flag
Sorry for the late response.
On Tue, 10 Apr 2018, Jonathan Gray wrote:
> On Thu, Apr 05, 2018 at 09:57:20PM +0200, Stefan Fritsch wrote:
> > Some em chips have a semaphore ("software flag") to synchronize access
> > to certain registers between OS and firmware (ME/AMT).
> >
> > Make the logic to get the flag match the logic in freebsd. This includes
> > higher timeouts and waiting for a previous unlock to complete before
> > trying a lock again.
>
> Shouldn't the printfs remain as DEBUGOUT() and DEBUGOUT2() as they are
> in FreeBSD?
I am pretty sure that things will break if the SW flag cannot be aquired
and I prefer to have meaningful error messages in that case. But I am fine
either way.
> > ---
> > sys/dev/pci/if_em_hw.c | 22 +++---
> > sys/dev/pci/if_em_hw.h | 2 ++
> > 2 files changed, 21 insertions(+), 3 deletions(-)
> >
> > diff --git sys/dev/pci/if_em_hw.c sys/dev/pci/if_em_hw.c
> > index e5084252c29..5bba83cbcd4 100644
> > --- sys/dev/pci/if_em_hw.c
> > +++ sys/dev/pci/if_em_hw.c
> > @@ -9613,9 +9613,21 @@ em_get_software_flag(struct em_hw *hw)
> > if (IS_ICH8(hw->mac_type)) {
> > while (timeout) {
> > extcnf_ctrl = E1000_READ_REG(hw, EXTCNF_CTRL);
> > - extcnf_ctrl |= E1000_EXTCNF_CTRL_SWFLAG;
> > - E1000_WRITE_REG(hw, EXTCNF_CTRL, extcnf_ctrl);
> > + if (!(extcnf_ctrl & E1000_EXTCNF_CTRL_SWFLAG))
> > + break;
> > + msec_delay_irq(1);
> > + timeout--;
> > + }
> > + if (!timeout) {
> > + printf("%s: SW has already locked the resource?\n",
> > + __func__);
> > + return -E1000_ERR_CONFIG;
> > + }
> > + timeout = SW_FLAG_TIMEOUT;
> > + extcnf_ctrl |= E1000_EXTCNF_CTRL_SWFLAG;
> > + E1000_WRITE_REG(hw, EXTCNF_CTRL, extcnf_ctrl);
> >
> > + while (timeout) {
> > extcnf_ctrl = E1000_READ_REG(hw, EXTCNF_CTRL);
> > if (extcnf_ctrl & E1000_EXTCNF_CTRL_SWFLAG)
> > break;
> > @@ -9624,7 +9636,11 @@ em_get_software_flag(struct em_hw *hw)
> > }
> >
> > if (!timeout) {
> > - DEBUGOUT("FW or HW locks the resource too long.\n");
> > + printf("Failed to acquire the semaphore, FW or HW "
> > + "has it: FWSM=0x%8.8x EXTCNF_CTRL=0x%8.8x)\n",
> > + E1000_READ_REG(hw, FWSM), extcnf_ctrl);
> > + extcnf_ctrl &= ~E1000_EXTCNF_CTRL_SWFLAG;
> > + E1000_WRITE_REG(hw, EXTCNF_CTRL, extcnf_ctrl);
> > return -E1000_ERR_CONFIG;
> > }
> > }
> > diff --git sys/dev/pci/if_em_hw.h sys/dev/pci/if_em_hw.h
> > index 5e415e60a34..71dc91e5582 100644
> > --- sys/dev/pci/if_em_hw.h
> > +++ sys/dev/pci/if_em_hw.h
> > @@ -2755,6 +2755,8 @@ struct em_host_command_info {
> > #define AUTO_READ_DONE_TIMEOUT 10
> > /* Number of milliseconds we wait for PHY configuration done after MAC
> > reset */
> > #define PHY_CFG_TIMEOUT 100
> > +/* SW Semaphore flag timeout in ms */
> > +#define SW_FLAG_TIMEOUT1000
> >
> > #define E1000_TX_BUFFER_SIZE ((uint32_t)1514)
> >
> > --
> > 2.13.0
> >
>
>
Re: [PATCH 4/7] em: Improve access logic for software flag
On Thu, Apr 05, 2018 at 09:57:20PM +0200, Stefan Fritsch wrote:
> Some em chips have a semaphore ("software flag") to synchronize access
> to certain registers between OS and firmware (ME/AMT).
>
> Make the logic to get the flag match the logic in freebsd. This includes
> higher timeouts and waiting for a previous unlock to complete before
> trying a lock again.
Shouldn't the printfs remain as DEBUGOUT() and DEBUGOUT2() as they are
in FreeBSD?
> ---
> sys/dev/pci/if_em_hw.c | 22 +++---
> sys/dev/pci/if_em_hw.h | 2 ++
> 2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git sys/dev/pci/if_em_hw.c sys/dev/pci/if_em_hw.c
> index e5084252c29..5bba83cbcd4 100644
> --- sys/dev/pci/if_em_hw.c
> +++ sys/dev/pci/if_em_hw.c
> @@ -9613,9 +9613,21 @@ em_get_software_flag(struct em_hw *hw)
> if (IS_ICH8(hw->mac_type)) {
> while (timeout) {
> extcnf_ctrl = E1000_READ_REG(hw, EXTCNF_CTRL);
> - extcnf_ctrl |= E1000_EXTCNF_CTRL_SWFLAG;
> - E1000_WRITE_REG(hw, EXTCNF_CTRL, extcnf_ctrl);
> + if (!(extcnf_ctrl & E1000_EXTCNF_CTRL_SWFLAG))
> + break;
> + msec_delay_irq(1);
> + timeout--;
> + }
> + if (!timeout) {
> + printf("%s: SW has already locked the resource?\n",
> + __func__);
> + return -E1000_ERR_CONFIG;
> + }
> + timeout = SW_FLAG_TIMEOUT;
> + extcnf_ctrl |= E1000_EXTCNF_CTRL_SWFLAG;
> + E1000_WRITE_REG(hw, EXTCNF_CTRL, extcnf_ctrl);
>
> + while (timeout) {
> extcnf_ctrl = E1000_READ_REG(hw, EXTCNF_CTRL);
> if (extcnf_ctrl & E1000_EXTCNF_CTRL_SWFLAG)
> break;
> @@ -9624,7 +9636,11 @@ em_get_software_flag(struct em_hw *hw)
> }
>
> if (!timeout) {
> - DEBUGOUT("FW or HW locks the resource too long.\n");
> + printf("Failed to acquire the semaphore, FW or HW "
> + "has it: FWSM=0x%8.8x EXTCNF_CTRL=0x%8.8x)\n",
> + E1000_READ_REG(hw, FWSM), extcnf_ctrl);
> + extcnf_ctrl &= ~E1000_EXTCNF_CTRL_SWFLAG;
> + E1000_WRITE_REG(hw, EXTCNF_CTRL, extcnf_ctrl);
> return -E1000_ERR_CONFIG;
> }
> }
> diff --git sys/dev/pci/if_em_hw.h sys/dev/pci/if_em_hw.h
> index 5e415e60a34..71dc91e5582 100644
> --- sys/dev/pci/if_em_hw.h
> +++ sys/dev/pci/if_em_hw.h
> @@ -2755,6 +2755,8 @@ struct em_host_command_info {
> #define AUTO_READ_DONE_TIMEOUT 10
> /* Number of milliseconds we wait for PHY configuration done after MAC reset
> */
> #define PHY_CFG_TIMEOUT 100
> +/* SW Semaphore flag timeout in ms */
> +#define SW_FLAG_TIMEOUT 1000
>
> #define E1000_TX_BUFFER_SIZE ((uint32_t)1514)
>
> --
> 2.13.0
>
[PATCH 4/7] em: Improve access logic for software flag
Some em chips have a semaphore ("software flag") to synchronize access
to certain registers between OS and firmware (ME/AMT).
Make the logic to get the flag match the logic in freebsd. This includes
higher timeouts and waiting for a previous unlock to complete before
trying a lock again.
---
sys/dev/pci/if_em_hw.c | 22 +++---
sys/dev/pci/if_em_hw.h | 2 ++
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git sys/dev/pci/if_em_hw.c sys/dev/pci/if_em_hw.c
index e5084252c29..5bba83cbcd4 100644
--- sys/dev/pci/if_em_hw.c
+++ sys/dev/pci/if_em_hw.c
@@ -9613,9 +9613,21 @@ em_get_software_flag(struct em_hw *hw)
if (IS_ICH8(hw->mac_type)) {
while (timeout) {
extcnf_ctrl = E1000_READ_REG(hw, EXTCNF_CTRL);
- extcnf_ctrl |= E1000_EXTCNF_CTRL_SWFLAG;
- E1000_WRITE_REG(hw, EXTCNF_CTRL, extcnf_ctrl);
+ if (!(extcnf_ctrl & E1000_EXTCNF_CTRL_SWFLAG))
+ break;
+ msec_delay_irq(1);
+ timeout--;
+ }
+ if (!timeout) {
+ printf("%s: SW has already locked the resource?\n",
+ __func__);
+ return -E1000_ERR_CONFIG;
+ }
+ timeout = SW_FLAG_TIMEOUT;
+ extcnf_ctrl |= E1000_EXTCNF_CTRL_SWFLAG;
+ E1000_WRITE_REG(hw, EXTCNF_CTRL, extcnf_ctrl);
+ while (timeout) {
extcnf_ctrl = E1000_READ_REG(hw, EXTCNF_CTRL);
if (extcnf_ctrl & E1000_EXTCNF_CTRL_SWFLAG)
break;
@@ -9624,7 +9636,11 @@ em_get_software_flag(struct em_hw *hw)
}
if (!timeout) {
- DEBUGOUT("FW or HW locks the resource too long.\n");
+ printf("Failed to acquire the semaphore, FW or HW "
+ "has it: FWSM=0x%8.8x EXTCNF_CTRL=0x%8.8x)\n",
+ E1000_READ_REG(hw, FWSM), extcnf_ctrl);
+ extcnf_ctrl &= ~E1000_EXTCNF_CTRL_SWFLAG;
+ E1000_WRITE_REG(hw, EXTCNF_CTRL, extcnf_ctrl);
return -E1000_ERR_CONFIG;
}
}
diff --git sys/dev/pci/if_em_hw.h sys/dev/pci/if_em_hw.h
index 5e415e60a34..71dc91e5582 100644
--- sys/dev/pci/if_em_hw.h
+++ sys/dev/pci/if_em_hw.h
@@ -2755,6 +2755,8 @@ struct em_host_command_info {
#define AUTO_READ_DONE_TIMEOUT 10
/* Number of milliseconds we wait for PHY configuration done after MAC reset */
#define PHY_CFG_TIMEOUT 100
+/* SW Semaphore flag timeout in ms */
+#define SW_FLAG_TIMEOUT1000
#define E1000_TX_BUFFER_SIZE ((uint32_t)1514)
--
2.13.0
