[PATCH net-next]r8169: Serialise some operations from open close

[PATCH net-next]r8169: Serialise some operations from open close

[Corcodel Marian]

This is a multi-part message in MIME format.
--------------mine-boundary-string
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit


Serialize some operations from open close function especial when write
read from main structure(struct rtl8169_private).Ensure that alls op is
executed.On slow proc this is good.
 Committer: Corcodel Marian <corcodel.marian@gmail.com>

 On branch master
 Your branch is ahead of 'origin/master' by 14 commits.
   (use "git push" to publish your local commits)

 Changes to be committed:
	modified:   drivers/net/ethernet/realtek/r8169.c

Signed-off-by: Corcodel Marian <asu@192-168-0-3.rdsnet.ro>
---
 drivers/net/ethernet/realtek/r8169.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)


--------------mine-boundary-string
Content-Type: text/x-patch; name="0001-Signed-off-by-Corcodel-Marian-corcodel.marian-gmail..patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment; filename="0001-Signed-off-by-Corcodel-Marian-corcodel.marian-gmail..patch"

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 410c1ee..be67873 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -7555,11 +7555,13 @@ static int rtl8169_close(struct net_device *dev)
 
 	rtl_lock_work(tp);
 	clear_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags);
-
-	rtl8169_down(dev);
 	rtl_unlock_work(tp);
 
+	rtl8169_down(dev);
+	//rtl_unlock_work(tp);
+        rtl_lock_work(tp);
 	cancel_work_sync(&tp->wk.work);
+	rtl_unlock_work(tp);
 
 	free_irq(pdev->irq, dev);
 
@@ -7567,9 +7569,10 @@ static int rtl8169_close(struct net_device *dev)
 			  tp->RxPhyAddr);
 	dma_free_coherent(&pdev->dev, R8169_TX_RING_BYTES, tp->TxDescArray,
 			  tp->TxPhyAddr);
+	rtl_lock_work(tp);
 	tp->TxDescArray = NULL;
 	tp->RxDescArray = NULL;
-
+        rtl_unlock_work(tp);
 	pm_runtime_put_sync(&pdev->dev);
 
 	return 0;
@@ -7611,10 +7614,12 @@ static int rtl_open(struct net_device *dev)
 	if (retval < 0)
 		goto err_free_rx_1;
 
+        rtl_lock_work(tp);
 	INIT_WORK(&tp->wk.work, rtl_task);
-
+        rtl_unlock_work(tp);
 	smp_mb();
 
+        rtl_lock_work(tp);
 	rtl_request_firmware(tp);
 
 	retval = request_irq(pdev->irq, rtl8169_interrupt,
@@ -7623,7 +7628,7 @@ static int rtl_open(struct net_device *dev)
 	if (retval < 0)
 		goto err_release_fw_2;
 
-	rtl_lock_work(tp);
+	//rtl_lock_work(tp);
 
 	set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags);
 
@@ -7634,17 +7639,21 @@ static int rtl_open(struct net_device *dev)
        //__rtl8169_set_features(dev, dev->features);
 
 	rtl_pll_power_up(tp);
+	rtl_unlock_work(tp);
 
 	rtl_hw_start(dev);
 
 	netif_start_queue(dev);
 
-	rtl_unlock_work(tp);
-
+	rtl_lock_work(tp);
 	tp->saved_wolopts = 0;
-	pm_runtime_put_noidle(&pdev->dev);
+	rtl_unlock_work(tp);
 
+	pm_runtime_put_noidle(&pdev->dev)
+		;
+        rtl_lock_work(tp);
 	rtl8169_check_link_status(dev, tp, ioaddr);
+	rtl_unlock_work(tp);
 out:
 	return retval;
 

--------------mine-boundary-string--

Re: [PATCH net-next]r8169: Serialise some operations from open close

[David Miller]

Please do not post patches as attachments.

Instead, inline them as unmolested ASCII text in the main message
body along with your commit message.

>From Documentation/email-clients.txt:

====================
General Preferences
----------------------------------------------------------------------
Patches for the Linux kernel are submitted via email, preferably as
inline text in the body of the email.  Some maintainers accept
attachments, but then the attachments should have content-type
"text/plain".  However, attachments are generally frowned upon because
it makes quoting portions of the patch more difficult in the patch
review process.
====================

Re: [PATCH net-next]r8169: Serialise some operations from open close

[Francois Romieu]

Corcodel Marian <corcodel.marian@gmail.com> :
[...]
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 410c1ee..be67873 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -7555,11 +7555,13 @@ static int rtl8169_close(struct net_device *dev)
>  
>  	rtl_lock_work(tp);
>  	clear_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags);
> -
> -	rtl8169_down(dev);
>  	rtl_unlock_work(tp);
>  
> +	rtl8169_down(dev);
> +	//rtl_unlock_work(tp);
> +        rtl_lock_work(tp);
>  	cancel_work_sync(&tp->wk.work);
> +	rtl_unlock_work(tp);
>  
>  	free_irq(pdev->irq, dev);

tab/spaces mix. Stray commented code.

rtl_lock_work/cancel_work_sync/rtl_unlock_work makes no sense.

[...]
> @@ -7611,10 +7614,12 @@ static int rtl_open(struct net_device *dev)
>  	if (retval < 0)
>  		goto err_free_rx_1;
>  
> +        rtl_lock_work(tp);
>  	INIT_WORK(&tp->wk.work, rtl_task);
> -
> +        rtl_unlock_work(tp);
>  	smp_mb();

?

Irq has not been requested : this code isn't racing with NAPI.

I don't see either how it could race with a runtime resume handler.

Care to elaborate what it is supposed to race with ?

>  
> +        rtl_lock_work(tp);
>  	rtl_request_firmware(tp);
>  
>  	retval = request_irq(pdev->irq, rtl8169_interrupt,
> @@ -7623,7 +7628,7 @@ static int rtl_open(struct net_device *dev)
>  	if (retval < 0)
>  		goto err_release_fw_2;

You forgot to balance rtl_lock_work after the err_release_fw_2 label.

:o/

-- 
Ueimor

Re: [PATCH net-next]r8169: Serialise some operations from open close

[Marian Corcodel]

This it.On me rtl 8101/8102 start now an full duplex with maximum
speed , some times need to restart autoneg in order to start .After
mutex_init access on main structure(struct rtl8169_private)  have only
on mutex_lock , mutex_unlock.

2015-07-15 2:40 GMT+03:00 Francois Romieu <romieu@fr.zoreil.com>:
> Corcodel Marian <corcodel.marian@gmail.com> :
> [...]
>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
>> index 410c1ee..be67873 100644
>> --- a/drivers/net/ethernet/realtek/r8169.c
>> +++ b/drivers/net/ethernet/realtek/r8169.c
>> @@ -7555,11 +7555,13 @@ static int rtl8169_close(struct net_device *dev)
>>
>>       rtl_lock_work(tp);
>>       clear_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags);
>> -
>> -     rtl8169_down(dev);
>>       rtl_unlock_work(tp);
>>
>> +     rtl8169_down(dev);
>> +     //rtl_unlock_work(tp);
>> +        rtl_lock_work(tp);
>>       cancel_work_sync(&tp->wk.work);
>> +     rtl_unlock_work(tp);
>>
>>       free_irq(pdev->irq, dev);
>
> tab/spaces mix. Stray commented code.
>
> rtl_lock_work/cancel_work_sync/rtl_unlock_work makes no sense.
>
> [...]
>> @@ -7611,10 +7614,12 @@ static int rtl_open(struct net_device *dev)
>>       if (retval < 0)
>>               goto err_free_rx_1;
>>
>> +        rtl_lock_work(tp);
>>       INIT_WORK(&tp->wk.work, rtl_task);
>> -
>> +        rtl_unlock_work(tp);
>>       smp_mb();
>
> ?
>
> Irq has not been requested : this code isn't racing with NAPI.
>
> I don't see either how it could race with a runtime resume handler.
>
> Care to elaborate what it is supposed to race with ?
>
>>
>> +        rtl_lock_work(tp);
>>       rtl_request_firmware(tp);
>>
>>       retval = request_irq(pdev->irq, rtl8169_interrupt,
>> @@ -7623,7 +7628,7 @@ static int rtl_open(struct net_device *dev)
>>       if (retval < 0)
>>               goto err_release_fw_2;
>
> You forgot to balance rtl_lock_work after the err_release_fw_2 label.
>
> :o/
>
> --
> Ueimor

[PATCH net-next 1/1] e1000: remove dead e1000_init_eeprom_params calls.

[Francois Romieu]

The device probe method e1000_probe calls e1000_init_eeprom_params
itself so there's no reason to call it again from e1000_do_write_eeprom
or e1000_do_read_eeprom.

The sentence above assumes that e1000_init_eeprom_params is effective
but it's mostly dependant on "hw->mac_type": safe as e1000_probe bails
out early if it can't set mac_type (see e1000_init_hw_struct, then
e1000_set_mac_type).

Btw, if effective, the removed paths would had been deadlock prone when
e1000_eeprom_spi was set:
-> e1000_write_eeprom (takes e1000_eeprom_lock)
   -> e1000_do_write_eeprom
      -> e1000_init_eeprom_params
         -> e1000_read_eeprom (takes e1000_eeprom_lock)

(same narrative with e1000_read_eeprom -> e1000_do_read_eeprom etc.)

As a final note, the candidate deadlock above can't happen in e1000_probe
due to the way eeprom->word_size is set / tested.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---

Untested. I have found it while looking at Joern's patch.

 drivers/net/ethernet/intel/e1000/e1000_hw.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000/e1000_hw.c b/drivers/net/ethernet/intel/e1000/e1000_hw.c
index 45c8c864..b1af0d6 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_hw.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_hw.c
@@ -3900,10 +3900,6 @@ static s32 e1000_do_read_eeprom(struct e1000_hw *hw, u16 offset, u16 words,
 		return E1000_SUCCESS;
 	}
 
-	/* If eeprom is not yet detected, do so now */
-	if (eeprom->word_size == 0)
-		e1000_init_eeprom_params(hw);
-
 	/* A check for invalid values:  offset too large, too many words, and
 	 * not enough words.
 	 */
@@ -4074,10 +4070,6 @@ static s32 e1000_do_write_eeprom(struct e1000_hw *hw, u16 offset, u16 words,
 		return E1000_SUCCESS;
 	}
 
-	/* If eeprom is not yet detected, do so now */
-	if (eeprom->word_size == 0)
-		e1000_init_eeprom_params(hw);
-
 	/* A check for invalid values:  offset too large, too many words, and
 	 * not enough words.
 	 */
-- 
2.4.3

Re: [PATCH net-next 1/1] e1000: remove dead e1000_init_eeprom_params calls.

[Jeff Kirsher]

On Fri, Jul 24, 2015 at 2:40 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> The device probe method e1000_probe calls e1000_init_eeprom_params
> itself so there's no reason to call it again from e1000_do_write_eeprom
> or e1000_do_read_eeprom.
>
> The sentence above assumes that e1000_init_eeprom_params is effective
> but it's mostly dependant on "hw->mac_type": safe as e1000_probe bails
> out early if it can't set mac_type (see e1000_init_hw_struct, then
> e1000_set_mac_type).
>
> Btw, if effective, the removed paths would had been deadlock prone when
> e1000_eeprom_spi was set:
> -> e1000_write_eeprom (takes e1000_eeprom_lock)
>    -> e1000_do_write_eeprom
>       -> e1000_init_eeprom_params
>          -> e1000_read_eeprom (takes e1000_eeprom_lock)
>
> (same narrative with e1000_read_eeprom -> e1000_do_read_eeprom etc.)
>
> As a final note, the candidate deadlock above can't happen in e1000_probe
> due to the way eeprom->word_size is set / tested.
>
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
> ---
>
> Untested. I have found it while looking at Joern's patch.
>
>  drivers/net/ethernet/intel/e1000/e1000_hw.c | 8 --------
>  1 file changed, 8 deletions(-)

Can you please send this to intel-wired-lan@lists.osuosl.org mailing
list?  That is the mailing list created/used for these patches.  It
also helps me out by adding your patch to our patchworks project for
patches against Intel wired drivers.

Thanks in advance, sorry for the delayed response, was on vacation last week.