[net-queue] ixgbe: Move ipsec init function to before reset call

This patch moves the ipsec init function in ixgbe_sw_init. This way it is a
bit more consistent with the placement of similar initialization functions
and is placed before the reset_hw call which should allow us to clean up
any link issues that may be introduced by the fact that we force the link
up if somehow the device had ipsec still enabled before the driver was
loaded.

Fixes: 49a94d74d948 ("ixgbe: add ipsec engine start and stop routines")
Reported-by: Andre Tomt <andre@tomt.net>
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

I haven't had a chance to test this myself yet since I don't have a system
that is reproducing this issue. I am including Andre in the "To:" line in
the hopes that perhaps he can test this to see if it resolves the issue.

 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

On 6/4/2018 9:24 AM, Alexander Duyck wrote:
> This patch moves the ipsec init function in ixgbe_sw_init. This way it is a
> bit more consistent with the placement of similar initialization functions
> and is placed before the reset_hw call which should allow us to clean up
> any link issues that may be introduced by the fact that we force the link
> up if somehow the device had ipsec still enabled before the driver was
> loaded.
> 
> Fixes: 49a94d74d948 ("ixgbe: add ipsec engine start and stop routines")
> Reported-by: Andre Tomt <andre@tomt.net>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---

NAK - this won't work for the simple reason that the netdev feature 
flags that ixgbe_init_ipsec_offload() sets will get bashed later when 
probe() starts setting netdev feature flags.

The call to stop the engine in the init is probably unnecessary, I 
really put it there to be sure that no crap was left lying around from 
before the driver was started.  This can probably be removed to address 
Alex's timing worries.

We really need a way to check the SEC_EN or ENCRYPTION_EN pins to see if 
the chip has it supported.  The alternative is to do a quick check to 
see if we can write to SECTXCTRL.SECTX_DIS and SECRXCTRL.SECRX_DIS to 
clear those bits.  Since I don't know how to check the pin values, I'll 
work up a sequence to check the SECTXCTRL bits for writability.

sln

> 
> I haven't had a chance to test this myself yet since I don't have a system
> that is reproducing this issue. I am including Andre in the "To:" line in
> the hopes that perhaps he can test this to see if it resolves the issue.
> 
>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index c42498d..78e7516 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -6128,6 +6128,7 @@ static int ixgbe_sw_init(struct ixgbe_adapter *adapter,
>   #ifdef CONFIG_IXGBE_DCB
>   	ixgbe_init_dcb(adapter);
>   #endif
> +	ixgbe_init_ipsec_offload(adapter);
>   
>   	/* default flow control settings */
>   	hw->fc.requested_mode = ixgbe_fc_full;
> @@ -10537,8 +10538,6 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   					 NETIF_F_FCOE_MTU;
>   	}
>   #endif /* IXGBE_FCOE */
> -	ixgbe_init_ipsec_offload(adapter);
> -
>   	if (adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE)
>   		netdev->hw_features |= NETIF_F_LRO;
>   	if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED)
>

On Mon, Jun 4, 2018 at 10:34 AM, Shannon Nelson
<shannon.nelson@oracle.com> wrote:
> On 6/4/2018 9:24 AM, Alexander Duyck wrote:
>>
>> This patch moves the ipsec init function in ixgbe_sw_init. This way it is
>> a
>> bit more consistent with the placement of similar initialization functions
>> and is placed before the reset_hw call which should allow us to clean up
>> any link issues that may be introduced by the fact that we force the link
>> up if somehow the device had ipsec still enabled before the driver was
>> loaded.
>>
>> Fixes: 49a94d74d948 ("ixgbe: add ipsec engine start and stop routines")
>> Reported-by: Andre Tomt <andre@tomt.net>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>> ---
>
>
> NAK - this won't work for the simple reason that the netdev feature flags
> that ixgbe_init_ipsec_offload() sets will get bashed later when probe()
> starts setting netdev feature flags.

Yeah, I realized that after I started doing any testing. I was already
working on a v2 to address that as we could pull the feature bit
setting out and place it with the other features and only enable it
based on if adapter->ipsec is set or not.

> The call to stop the engine in the init is probably unnecessary, I really
> put it there to be sure that no crap was left lying around from before the
> driver was started.  This can probably be removed to address Alex's timing
> worries.

Actually I think we just need to look at moving this to a spot before
the reset_hw call. My thought is if we place it here, and add the bit
for setting the feature bits later we can probably resolve most of the
issues.

Also it just occurred to me that we will always report link as being
down where the function is called since the watchdog hasn't had a
chance to start yet.

> We really need a way to check the SEC_EN or ENCRYPTION_EN pins to see if the
> chip has it supported.  The alternative is to do a quick check to see if we
> can write to SECTXCTRL.SECTX_DIS and SECRXCTRL.SECRX_DIS to clear those
> bits.  Since I don't know how to check the pin values, I'll work up a
> sequence to check the SECTXCTRL bits for writability.
>
> sln

Actually I'm wondering if we couldn't just get away with testing the
SECTXCTRL.TX_DIS and SECRXCTRL.RX_DIS bits instead. If I am
understanding your function correctly I think it is leaving those bits
set, and from what I can tell it looks like in the case of hardware
with the functionality disabled those bits might be read-only. If so
that might be a good way for us to test the functionality as disabled
by us, versus disabled by that bit may differ in those bits.

- Alex

On Mon, Jun 4, 2018 at 11:07 AM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Mon, Jun 4, 2018 at 10:34 AM, Shannon Nelson
> <shannon.nelson@oracle.com> wrote:
>> On 6/4/2018 9:24 AM, Alexander Duyck wrote:
>>>
>>> This patch moves the ipsec init function in ixgbe_sw_init. This way it is
>>> a
>>> bit more consistent with the placement of similar initialization functions
>>> and is placed before the reset_hw call which should allow us to clean up
>>> any link issues that may be introduced by the fact that we force the link
>>> up if somehow the device had ipsec still enabled before the driver was
>>> loaded.
>>>
>>> Fixes: 49a94d74d948 ("ixgbe: add ipsec engine start and stop routines")
>>> Reported-by: Andre Tomt <andre@tomt.net>
>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>>> ---
>>
>>
>> NAK - this won't work for the simple reason that the netdev feature flags
>> that ixgbe_init_ipsec_offload() sets will get bashed later when probe()
>> starts setting netdev feature flags.
>
> Yeah, I realized that after I started doing any testing. I was already
> working on a v2 to address that as we could pull the feature bit
> setting out and place it with the other features and only enable it
> based on if adapter->ipsec is set or not.

Jeff,

Please drop this patch. I will work with Shannon to make sure we have
a more complete solution.

Thanks.

- Alex

On Mon, 2018-06-04 at 11:14 -0700, Alexander Duyck wrote:
> On Mon, Jun 4, 2018 at 11:07 AM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
> > On Mon, Jun 4, 2018 at 10:34 AM, Shannon Nelson
> > <shannon.nelson@oracle.com> wrote:
> > > On 6/4/2018 9:24 AM, Alexander Duyck wrote:
> > > > 
> > > > This patch moves the ipsec init function in ixgbe_sw_init. This
> > > > way it is
> > > > a
> > > > bit more consistent with the placement of similar
> > > > initialization functions
> > > > and is placed before the reset_hw call which should allow us to
> > > > clean up
> > > > any link issues that may be introduced by the fact that we
> > > > force the link
> > > > up if somehow the device had ipsec still enabled before the
> > > > driver was
> > > > loaded.
> > > > 
> > > > Fixes: 49a94d74d948 ("ixgbe: add ipsec engine start and stop
> > > > routines")
> > > > Reported-by: Andre Tomt <andre@tomt.net>
> > > > Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> > > > ---
> > > 
> > > 
> > > NAK - this won't work for the simple reason that the netdev
> > > feature flags
> > > that ixgbe_init_ipsec_offload() sets will get bashed later when
> > > probe()
> > > starts setting netdev feature flags.
> > 
> > Yeah, I realized that after I started doing any testing. I was
> > already
> > working on a v2 to address that as we could pull the feature bit
> > setting out and place it with the other features and only enable it
> > based on if adapter->ipsec is set or not.
> 
> Jeff,
> 
> Please drop this patch. I will work with Shannon to make sure we have
> a more complete solution.

Already done.

On 6/4/2018 11:07 AM, Alexander Duyck wrote:
> On Mon, Jun 4, 2018 at 10:34 AM, Shannon Nelson
> <shannon.nelson@oracle.com> wrote:
>> On 6/4/2018 9:24 AM, Alexander Duyck wrote:
>>>
>>> This patch moves the ipsec init function in ixgbe_sw_init. This way it is
>>> a
>>> bit more consistent with the placement of similar initialization functions
>>> and is placed before the reset_hw call which should allow us to clean up
>>> any link issues that may be introduced by the fact that we force the link
>>> up if somehow the device had ipsec still enabled before the driver was
>>> loaded.
>>>
>>> Fixes: 49a94d74d948 ("ixgbe: add ipsec engine start and stop routines")
>>> Reported-by: Andre Tomt <andre@tomt.net>
>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>>> ---
>>
>>
>> NAK - this won't work for the simple reason that the netdev feature flags
>> that ixgbe_init_ipsec_offload() sets will get bashed later when probe()
>> starts setting netdev feature flags.
> 
> Yeah, I realized that after I started doing any testing. I was already
> working on a v2 to address that as we could pull the feature bit
> setting out and place it with the other features and only enable it
> based on if adapter->ipsec is set or not.
> 
>> The call to stop the engine in the init is probably unnecessary, I really
>> put it there to be sure that no crap was left lying around from before the
>> driver was started.  This can probably be removed to address Alex's timing
>> worries.
> 
> Actually I think we just need to look at moving this to a spot before
> the reset_hw call. My thought is if we place it here, and add the bit
> for setting the feature bits later we can probably resolve most of the
> issues.
> 
> Also it just occurred to me that we will always report link as being
> down where the function is called since the watchdog hasn't had a
> chance to start yet.
> 
>> We really need a way to check the SEC_EN or ENCRYPTION_EN pins to see if the
>> chip has it supported.  The alternative is to do a quick check to see if we
>> can write to SECTXCTRL.SECTX_DIS and SECRXCTRL.SECRX_DIS to clear those
>> bits.  Since I don't know how to check the pin values, I'll work up a
>> sequence to check the SECTXCTRL bits for writability.
>>
>> sln
> 
> Actually I'm wondering if we couldn't just get away with testing the
> SECTXCTRL.TX_DIS and SECRXCTRL.RX_DIS bits instead. If I am
> understanding your function correctly I think it is leaving those bits
> set, and from what I can tell it looks like in the case of hardware
> with the functionality disabled those bits might be read-only. If so
> that might be a good way for us to test the functionality as disabled
> by us, versus disabled by that bit may differ in those bits.

Yes - the bits are set when the engine is disabled, and they need to be 
cleared to run the encryption engine.  Accordign tot he spec, if the 
SEC_EN or ENCRYPTION_EN pins are set for "disable", then the DIS bits 
are not writable and can't be set to 0.  This is what I was thinking as 
well.  I was going to write a bit of code for this shortly.

sln

> 
> - Alex
>