[Intel-wired-lan] [PATCH net-next v1] e1000: remove unused and incorrect code

[Paul Menzel]

Dear Jesse,


Am 01.10.20 um 20:08 schrieb Jesse Brandeburg:
> On Thu, Oct 1, 2020 at 12:22 AM Paul Menzel <pmenzel at molgen.mpg.de> wrote:
>>> Fix this warning by removing the offending code and simplifying
>>> the routine to do exactly what it did before, no functional
>>> change.
>>
>> It looks like this fixes commit 1532ecea (e1000: drop dead pcie code
>> from e1000) removing support for e1000_82573?
> 
> It may, but that commit is from 2009, and since this code change
> actually doesn't fix any bug I didn't think it was a) worth davem
> targeting to net, b) worth the bots picking up for backporting to
> stable, as it is just waste of time, and maybe wouldn't be considered
> as important enough for stable.

You do not have to create a Fixes tag, but mentioning it in the commit 
message is well warranted, as it’s unclear, why the complicated code is 
there in the first place, and it took me several minutes to figure it out.

>>> Signed-off-by: Jesse Brandeburg <jesse.brandeburg at intel.com>
>>> ---
>>>
>>> NOTE: I don't recommend this go to stable as there is no functional
>>> change.
> 
> You'll note I attempted to address your comment with the above line
> even before you made it.

I didn’t write, it should be applied to the stable series.

>>> ---
>>>    drivers/net/ethernet/intel/e1000/e1000_hw.c | 10 +---------
>>>    1 file changed, 1 insertion(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/e1000/e1000_hw.c b/drivers/net/ethernet/intel/e1000/e1000_hw.c
>>> index 2120dacfd55c..c5d702543daa 100644
>>> --- a/drivers/net/ethernet/intel/e1000/e1000_hw.c
>>> +++ b/drivers/net/ethernet/intel/e1000/e1000_hw.c
>>> @@ -4403,17 +4403,9 @@ void e1000_write_vfta(struct e1000_hw *hw, u32 offset, u32 value)
>>>    static void e1000_clear_vfta(struct e1000_hw *hw)
>>
>> (Why is the diff also mentioning the function before `void
>> e1000_write_vfta()`?)
> 
> This is an artifact of how diff works, since the e1000_clear_vfta
> function was part of the diff context, the "previous function search"
> that diff does found write_vfta. There is no issue here, everything is
> working as it should.
> 
>>
>>>    {
>>>        u32 offset;
>>> -     u32 vfta_value = 0;
>>> -     u32 vfta_offset = 0;
>>> -     u32 vfta_bit_in_reg = 0;
>>>
>>>        for (offset = 0; offset < E1000_VLAN_FILTER_TBL_SIZE; offset++) {
>>> -             /* If the offset we want to clear is the same offset of the
>>> -              * manageability VLAN ID, then clear all bits except that of the
>>> -              * manageability unit
>>> -              */
>>> -             vfta_value = (offset == vfta_offset) ? vfta_bit_in_reg : 0;
>>> -             E1000_WRITE_REG_ARRAY(hw, VFTA, offset, vfta_value);
>>> +             E1000_WRITE_REG_ARRAY(hw, VFTA, offset, 0);
>>>                E1000_WRITE_FLUSH();
>>>        }
>>>    }
>>
>> It’d be great if you updated the commit description.
> 
> Thanks for your feedback, but in this case I don't think there is
> anything to change, I recommend we send the patch as-is. I appreciate
> your time spent reviewing!

Thank you for the reply.


Kind regards,

Paul