Re: [PATCH v4 11/16] mt76: sdio: extend sdio module to support CONNAC2

Re: [PATCH v4 11/16] mt76: sdio: extend sdio module to support CONNAC2

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

>[...]
>>
>> In the current driver, we can see we only created one Rx queue
>> (dev->q_rx with qid = 0) in mt76s_alloc_queues for processing all incoming packets including MCU events and wifi packets.
>>
>> And from the point of view of the device, mt7663s use the hardware
>> queue 0 for all MCU events and wifi packets; mt7921s use the hardware
>> queue 1 for all MCU events and wifi packets.
>>
>> So if we don't remap from hardware queue 1 to dev->q_rx[0] for mt7921s
>> to handle incoming packets, we will get the kernel panic on accessing the invalid pointer on dev->q_rx[1].
>>
>>	Sean
>>
>> >Regards,
>> >Lorenzo
>> >
>>
>> <snip>
>
>ok, what about doing something like the patch below?
>If it works for you, I will post a formal patch.

go ahead. that looks fine to me.

>
>Regards,
>Lorenzo
>

<snip>

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v4 11/16] mt76: sdio: extend sdio module to support CONNAC2

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

>[...]
>>
>> In the current driver, we can see we only created one Rx queue
>> (dev->q_rx with qid = 0) in mt76s_alloc_queues for processing all incoming packets including MCU events and wifi packets.
>>
>> And from the point of view of the device, mt7663s use the hardware
>> queue 0 for all MCU events and wifi packets; mt7921s use the hardware
>> queue 1 for all MCU events and wifi packets.
>>
>> So if we don't remap from hardware queue 1 to dev->q_rx[0] for mt7921s
>> to handle incoming packets, we will get the kernel panic on accessing the invalid pointer on dev->q_rx[1].
>>
>>	Sean
>>
>> >Regards,
>> >Lorenzo
>> >
>>
>> <snip>
>
>ok, what about doing something like the patch below?
>If it works for you, I will post a formal patch.

go ahead. that looks fine to me.

>
>Regards,
>Lorenzo
>

<snip>

Re: [PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

>> From: Sean Wang <sean.wang@mediatek.com>
>>
>>
>> <snip>
>>
>> >> >>
>> >> >> -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
>> >> >> -			if (!mt76s_txqs_empty(dev))
>> >> >> -				continue;
>> >> >> -			else
>> >> >> -				wake_up(&sdio->wait);
>> >> >> -		}
>> >> >>	} while (nframes > 0);
>> >> >>
>> >> >> +	if (test_bit(MT76_MCU_RESET, &dev->phy.state) &&
>> >> >> +	    mt76s_txqs_empty(dev))
>> >> >> +		wake_up(&sdio->wait);
>> >> >> +
>> >>
>> >> If doing so, mt76s_txqs_empty may not always be true because
>> >> enqueuing packets to q_tx or MCU command to q_mcu simultanenously
>> >> from the other contexts in different cpu is possible.
>> >>
>> >> It seemed to me we should check it for each iteration to guarantee
>> >> that we can wake up the one that is waiting for the all the queues are empty at some time.
>> >
>> >IIUC what we are interested here is there are no queued frames into the hw queues during suspend or reset, right?
>>
>> That is not completely true. Take the suspend procedure on mt7921s as an example.
>>
>> That should be "There are no queued frames into the hw queues right after mt76_connac_mcu_set_hif_suspend."
>>
>> The MCU data and WiFi are all handled in mt76s_txrx_worker so we
>> should synchronize all of the Tx queues are all empty and then handle
>> mt76_connac_mcu_set_hif_suspend to guarantee mt76_connac_mcu_set_hif_suspend is the last one to access the SDIO bus and there is no frame that accesses SDIO bus afterhand.
>
>ack, correct, "there are no queued frames into the hw queues right after mt76_connac_mcu_set_hif_suspend."
>What I mean is we are not really checking there are no frames in the hw queue here, but mt76 sdio has processed all the frames, got my point? maybe it is what we are looking for..

It seemed to me there is no a way to check if no frames in the hw queue for mt7921s

I think we can discuss the topic "it is ok move the check out of the loop" with another patch in the future

because it seemed not related to the patch.

>
>Regards,
>Lorenzo
>
>>
>> >
>> >>
>> >> >>	/* enable interrupt */
>> >> >>	sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
>> >> >>	sdio_release_host(sdio->func);
>> >> >>
>> >> >> Regards,
>> >> >> Lorenzo
>> >> >>
>> >> >> > --
>> >> >> > 2.25.1
>> >> >> >
>> >> >
>> >> >
>> >> >
>> >>
>> >

Re: [PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

>> From: Sean Wang <sean.wang@mediatek.com>
>>
>>
>> <snip>
>>
>> >> >>
>> >> >> -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
>> >> >> -			if (!mt76s_txqs_empty(dev))
>> >> >> -				continue;
>> >> >> -			else
>> >> >> -				wake_up(&sdio->wait);
>> >> >> -		}
>> >> >>	} while (nframes > 0);
>> >> >>
>> >> >> +	if (test_bit(MT76_MCU_RESET, &dev->phy.state) &&
>> >> >> +	    mt76s_txqs_empty(dev))
>> >> >> +		wake_up(&sdio->wait);
>> >> >> +
>> >>
>> >> If doing so, mt76s_txqs_empty may not always be true because
>> >> enqueuing packets to q_tx or MCU command to q_mcu simultanenously
>> >> from the other contexts in different cpu is possible.
>> >>
>> >> It seemed to me we should check it for each iteration to guarantee
>> >> that we can wake up the one that is waiting for the all the queues are empty at some time.
>> >
>> >IIUC what we are interested here is there are no queued frames into the hw queues during suspend or reset, right?
>>
>> That is not completely true. Take the suspend procedure on mt7921s as an example.
>>
>> That should be "There are no queued frames into the hw queues right after mt76_connac_mcu_set_hif_suspend."
>>
>> The MCU data and WiFi are all handled in mt76s_txrx_worker so we
>> should synchronize all of the Tx queues are all empty and then handle
>> mt76_connac_mcu_set_hif_suspend to guarantee mt76_connac_mcu_set_hif_suspend is the last one to access the SDIO bus and there is no frame that accesses SDIO bus afterhand.
>
>ack, correct, "there are no queued frames into the hw queues right after mt76_connac_mcu_set_hif_suspend."
>What I mean is we are not really checking there are no frames in the hw queue here, but mt76 sdio has processed all the frames, got my point? maybe it is what we are looking for..

It seemed to me there is no a way to check if no frames in the hw queue for mt7921s

I think we can discuss the topic "it is ok move the check out of the loop" with another patch in the future

because it seemed not related to the patch.

>
>Regards,
>Lorenzo
>
>>
>> >
>> >>
>> >> >>	/* enable interrupt */
>> >> >>	sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
>> >> >>	sdio_release_host(sdio->func);
>> >> >>
>> >> >> Regards,
>> >> >> Lorenzo
>> >> >>
>> >> >> > --
>> >> >> > 2.25.1
>> >> >> >
>> >> >
>> >> >
>> >> >
>> >>
>> >

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2610 bytes --]

> From: Sean Wang <sean.wang@mediatek.com>
> 
> >> From: Sean Wang <sean.wang@mediatek.com>
> >>
> >>
> >> <snip>
> >>
> >> >> >>
> >> >> >> -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
> >> >> >> -			if (!mt76s_txqs_empty(dev))
> >> >> >> -				continue;
> >> >> >> -			else
> >> >> >> -				wake_up(&sdio->wait);
> >> >> >> -		}
> >> >> >>	} while (nframes > 0);
> >> >> >>
> >> >> >> +	if (test_bit(MT76_MCU_RESET, &dev->phy.state) &&
> >> >> >> +	    mt76s_txqs_empty(dev))
> >> >> >> +		wake_up(&sdio->wait);
> >> >> >> +
> >> >>
> >> >> If doing so, mt76s_txqs_empty may not always be true because
> >> >> enqueuing packets to q_tx or MCU command to q_mcu simultanenously
> >> >> from the other contexts in different cpu is possible.
> >> >>
> >> >> It seemed to me we should check it for each iteration to guarantee
> >> >> that we can wake up the one that is waiting for the all the queues are empty at some time.
> >> >
> >> >IIUC what we are interested here is there are no queued frames into the hw queues during suspend or reset, right?
> >>
> >> That is not completely true. Take the suspend procedure on mt7921s as an example.
> >>
> >> That should be "There are no queued frames into the hw queues right after mt76_connac_mcu_set_hif_suspend."
> >>
> >> The MCU data and WiFi are all handled in mt76s_txrx_worker so we
> >> should synchronize all of the Tx queues are all empty and then handle
> >> mt76_connac_mcu_set_hif_suspend to guarantee mt76_connac_mcu_set_hif_suspend is the last one to access the SDIO bus and there is no frame that accesses SDIO bus afterhand.
> >
> >ack, correct, "there are no queued frames into the hw queues right after mt76_connac_mcu_set_hif_suspend."
> >What I mean is we are not really checking there are no frames in the hw queue here, but mt76 sdio has processed all the frames, got my point? maybe it is what we are looking for..
> 
> It seemed to me there is no a way to check if no frames in the hw queue for mt7921s
> 
> I think we can discuss the topic "it is ok move the check out of the loop" with another patch in the future
> 
> because it seemed not related to the patch.

sure, fine to me.

Regards,
Lorenzo

> 
> >
> >Regards,
> >Lorenzo
> >
> >>
> >> >
> >> >>
> >> >> >>	/* enable interrupt */
> >> >> >>	sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
> >> >> >>	sdio_release_host(sdio->func);
> >> >> >>
> >> >> >> Regards,
> >> >> >> Lorenzo
> >> >> >>
> >> >> >> > --
> >> >> >> > 2.25.1
> >> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >> >>
> >> >

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend

[Lorenzo Bianconi]

[-- Attachment #1.1: Type: text/plain, Size: 2610 bytes --]

> From: Sean Wang <sean.wang@mediatek.com>
> 
> >> From: Sean Wang <sean.wang@mediatek.com>
> >>
> >>
> >> <snip>
> >>
> >> >> >>
> >> >> >> -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
> >> >> >> -			if (!mt76s_txqs_empty(dev))
> >> >> >> -				continue;
> >> >> >> -			else
> >> >> >> -				wake_up(&sdio->wait);
> >> >> >> -		}
> >> >> >>	} while (nframes > 0);
> >> >> >>
> >> >> >> +	if (test_bit(MT76_MCU_RESET, &dev->phy.state) &&
> >> >> >> +	    mt76s_txqs_empty(dev))
> >> >> >> +		wake_up(&sdio->wait);
> >> >> >> +
> >> >>
> >> >> If doing so, mt76s_txqs_empty may not always be true because
> >> >> enqueuing packets to q_tx or MCU command to q_mcu simultanenously
> >> >> from the other contexts in different cpu is possible.
> >> >>
> >> >> It seemed to me we should check it for each iteration to guarantee
> >> >> that we can wake up the one that is waiting for the all the queues are empty at some time.
> >> >
> >> >IIUC what we are interested here is there are no queued frames into the hw queues during suspend or reset, right?
> >>
> >> That is not completely true. Take the suspend procedure on mt7921s as an example.
> >>
> >> That should be "There are no queued frames into the hw queues right after mt76_connac_mcu_set_hif_suspend."
> >>
> >> The MCU data and WiFi are all handled in mt76s_txrx_worker so we
> >> should synchronize all of the Tx queues are all empty and then handle
> >> mt76_connac_mcu_set_hif_suspend to guarantee mt76_connac_mcu_set_hif_suspend is the last one to access the SDIO bus and there is no frame that accesses SDIO bus afterhand.
> >
> >ack, correct, "there are no queued frames into the hw queues right after mt76_connac_mcu_set_hif_suspend."
> >What I mean is we are not really checking there are no frames in the hw queue here, but mt76 sdio has processed all the frames, got my point? maybe it is what we are looking for..
> 
> It seemed to me there is no a way to check if no frames in the hw queue for mt7921s
> 
> I think we can discuss the topic "it is ok move the check out of the loop" with another patch in the future
> 
> because it seemed not related to the patch.

sure, fine to me.

Regards,
Lorenzo

> 
> >
> >Regards,
> >Lorenzo
> >
> >>
> >> >
> >> >>
> >> >> >>	/* enable interrupt */
> >> >> >>	sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
> >> >> >>	sdio_release_host(sdio->func);
> >> >> >>
> >> >> >> Regards,
> >> >> >> Lorenzo
> >> >> >>
> >> >> >> > --
> >> >> >> > 2.25.1
> >> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >> >>
> >> >

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2087 bytes --]

> From: Sean Wang <sean.wang@mediatek.com>
> 
> 
> <snip>
> 
> >> >>
> >> >> -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
> >> >> -			if (!mt76s_txqs_empty(dev))
> >> >> -				continue;
> >> >> -			else
> >> >> -				wake_up(&sdio->wait);
> >> >> -		}
> >> >>	} while (nframes > 0);
> >> >>
> >> >> +	if (test_bit(MT76_MCU_RESET, &dev->phy.state) &&
> >> >> +	    mt76s_txqs_empty(dev))
> >> >> +		wake_up(&sdio->wait);
> >> >> +
> >>
> >> If doing so, mt76s_txqs_empty may not always be true because enqueuing
> >> packets to q_tx or MCU command to q_mcu simultanenously from the other
> >> contexts in different cpu is possible.
> >>
> >> It seemed to me we should check it for each iteration to guarantee
> >> that we can wake up the one that is waiting for the all the queues are empty at some time.
> >
> >IIUC what we are interested here is there are no queued frames into the hw queues during suspend or reset, right?
> 
> That is not completely true. Take the suspend procedure on mt7921s as an example.
> 
> That should be "There are no queued frames into the hw queues right after mt76_connac_mcu_set_hif_suspend."
> 
> The MCU data and WiFi are all handled in mt76s_txrx_worker so we should synchronize all of
> the Tx queues are all empty and then handle mt76_connac_mcu_set_hif_suspend to guarantee
> mt76_connac_mcu_set_hif_suspend is the last one to access the SDIO bus and there is no frame that accesses SDIO bus afterhand.

ack, correct, "there are no queued frames into the hw queues right after
mt76_connac_mcu_set_hif_suspend."
What I mean is we are not really checking there are no frames in the hw queue
here, but mt76 sdio has processed all the frames, got my point? maybe it is
what we are looking for..

Regards,
Lorenzo

> 
> >
> >>
> >> >>	/* enable interrupt */
> >> >>	sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
> >> >>	sdio_release_host(sdio->func);
> >> >>
> >> >> Regards,
> >> >> Lorenzo
> >> >>
> >> >> > --
> >> >> > 2.25.1
> >> >> >
> >> >
> >> >
> >> >
> >>
> >

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend

[Lorenzo Bianconi]

[-- Attachment #1.1: Type: text/plain, Size: 2087 bytes --]

> From: Sean Wang <sean.wang@mediatek.com>
> 
> 
> <snip>
> 
> >> >>
> >> >> -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
> >> >> -			if (!mt76s_txqs_empty(dev))
> >> >> -				continue;
> >> >> -			else
> >> >> -				wake_up(&sdio->wait);
> >> >> -		}
> >> >>	} while (nframes > 0);
> >> >>
> >> >> +	if (test_bit(MT76_MCU_RESET, &dev->phy.state) &&
> >> >> +	    mt76s_txqs_empty(dev))
> >> >> +		wake_up(&sdio->wait);
> >> >> +
> >>
> >> If doing so, mt76s_txqs_empty may not always be true because enqueuing
> >> packets to q_tx or MCU command to q_mcu simultanenously from the other
> >> contexts in different cpu is possible.
> >>
> >> It seemed to me we should check it for each iteration to guarantee
> >> that we can wake up the one that is waiting for the all the queues are empty at some time.
> >
> >IIUC what we are interested here is there are no queued frames into the hw queues during suspend or reset, right?
> 
> That is not completely true. Take the suspend procedure on mt7921s as an example.
> 
> That should be "There are no queued frames into the hw queues right after mt76_connac_mcu_set_hif_suspend."
> 
> The MCU data and WiFi are all handled in mt76s_txrx_worker so we should synchronize all of
> the Tx queues are all empty and then handle mt76_connac_mcu_set_hif_suspend to guarantee
> mt76_connac_mcu_set_hif_suspend is the last one to access the SDIO bus and there is no frame that accesses SDIO bus afterhand.

ack, correct, "there are no queued frames into the hw queues right after
mt76_connac_mcu_set_hif_suspend."
What I mean is we are not really checking there are no frames in the hw queue
here, but mt76 sdio has processed all the frames, got my point? maybe it is
what we are looking for..

Regards,
Lorenzo

> 
> >
> >>
> >> >>	/* enable interrupt */
> >> >>	sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
> >> >>	sdio_release_host(sdio->func);
> >> >>
> >> >> Regards,
> >> >> Lorenzo
> >> >>
> >> >> > --
> >> >> > 2.25.1
> >> >> >
> >> >
> >> >
> >> >
> >>
> >

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>


<snip>

>> >>
>> >> -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
>> >> -			if (!mt76s_txqs_empty(dev))
>> >> -				continue;
>> >> -			else
>> >> -				wake_up(&sdio->wait);
>> >> -		}
>> >>	} while (nframes > 0);
>> >>
>> >> +	if (test_bit(MT76_MCU_RESET, &dev->phy.state) &&
>> >> +	    mt76s_txqs_empty(dev))
>> >> +		wake_up(&sdio->wait);
>> >> +
>>
>> If doing so, mt76s_txqs_empty may not always be true because enqueuing
>> packets to q_tx or MCU command to q_mcu simultanenously from the other
>> contexts in different cpu is possible.
>>
>> It seemed to me we should check it for each iteration to guarantee
>> that we can wake up the one that is waiting for the all the queues are empty at some time.
>
>IIUC what we are interested here is there are no queued frames into the hw queues during suspend or reset, right?

That is not completely true. Take the suspend procedure on mt7921s as an example.

That should be "There are no queued frames into the hw queues right after mt76_connac_mcu_set_hif_suspend."

The MCU data and WiFi are all handled in mt76s_txrx_worker so we should synchronize all of
the Tx queues are all empty and then handle mt76_connac_mcu_set_hif_suspend to guarantee
mt76_connac_mcu_set_hif_suspend is the last one to access the SDIO bus and there is no frame that accesses SDIO bus afterhand.

>
>>
>> >>	/* enable interrupt */
>> >>	sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
>> >>	sdio_release_host(sdio->func);
>> >>
>> >> Regards,
>> >> Lorenzo
>> >>
>> >> > --
>> >> > 2.25.1
>> >> >
>> >
>> >
>> >
>>
>

Re: [PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>


<snip>

>> >>
>> >> -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
>> >> -			if (!mt76s_txqs_empty(dev))
>> >> -				continue;
>> >> -			else
>> >> -				wake_up(&sdio->wait);
>> >> -		}
>> >>	} while (nframes > 0);
>> >>
>> >> +	if (test_bit(MT76_MCU_RESET, &dev->phy.state) &&
>> >> +	    mt76s_txqs_empty(dev))
>> >> +		wake_up(&sdio->wait);
>> >> +
>>
>> If doing so, mt76s_txqs_empty may not always be true because enqueuing
>> packets to q_tx or MCU command to q_mcu simultanenously from the other
>> contexts in different cpu is possible.
>>
>> It seemed to me we should check it for each iteration to guarantee
>> that we can wake up the one that is waiting for the all the queues are empty at some time.
>
>IIUC what we are interested here is there are no queued frames into the hw queues during suspend or reset, right?

That is not completely true. Take the suspend procedure on mt7921s as an example.

That should be "There are no queued frames into the hw queues right after mt76_connac_mcu_set_hif_suspend."

The MCU data and WiFi are all handled in mt76s_txrx_worker so we should synchronize all of
the Tx queues are all empty and then handle mt76_connac_mcu_set_hif_suspend to guarantee
mt76_connac_mcu_set_hif_suspend is the last one to access the SDIO bus and there is no frame that accesses SDIO bus afterhand.

>
>>
>> >>	/* enable interrupt */
>> >>	sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
>> >>	sdio_release_host(sdio->func);
>> >>
>> >> Regards,
>> >> Lorenzo
>> >>
>> >> > --
>> >> > 2.25.1
>> >> >
>> >
>> >
>> >
>>
>

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 10067 bytes --]

> From: Sean Wang <sean.wang@mediatek.com>
> 
> >> > From: Sean Wang <sean.wang@mediatek.com>
> >> >
> >> > According to the MT7921S firmware, the cmd MCU_UNI_CMD_HIF_CTRL have
> >> > to be last MCU command to execute in suspend handler and all data
> >> > traffic have to be stopped before the cmd MCU_UNI_CMD_HIF_CTRL
> >> > starts as well in order that mt7921 can successfully fall into the deep sleep mode.
> >> >
> >> > Where we reuse the flag MT76_STATE_SUSPEND and avoid creating
> >> > another global flag to stop all of the traffic onto the SDIO bus.
> >> >
> >> > Fixes: 48fab5bbef40 ("mt76: mt7921: introduce mt7921s support")
> >> > Reported-by: Leon Yen <leon.yen@mediatek.com>
> >> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> >> > ---
> >> >  .../wireless/mediatek/mt76/mt76_connac_mcu.c  |  2 +-
> >> > .../net/wireless/mediatek/mt76/mt7921/main.c  |  3 --
> >> > .../net/wireless/mediatek/mt76/mt7921/sdio.c  | 34 ++++++++++++-------
> >> >  drivers/net/wireless/mediatek/mt76/sdio.c     |  3 +-
> >> >  .../net/wireless/mediatek/mt76/sdio_txrx.c    |  3 +-
> >> >  5 files changed, 27 insertions(+), 18 deletions(-)
> >> >
> >> > diff --git a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> >> > b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> >> > index 26b4b875dcc0..61c4c86e79c8 100644
> >> > --- a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> >> > +++ b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> >> > @@ -2461,7 +2461,7 @@ void mt76_connac_mcu_set_suspend_iter(void *priv, u8 *mac,
> >> >				      struct ieee80211_vif *vif)  {
> >> >	struct mt76_phy *phy = priv;
> >> > -	bool suspend = test_bit(MT76_STATE_SUSPEND, &phy->state);
> >> > +	bool suspend = !test_bit(MT76_STATE_RUNNING, &phy->state);
> >> >	struct ieee80211_hw *hw = phy->hw;
> >> >	struct cfg80211_wowlan *wowlan = hw->wiphy->wowlan_config;
> >> >	int i;
> >> > diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> >> > b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> >> > index e022251b4006..0b2a6b7f22ea 100644
> >> > --- a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> >> > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> >> > @@ -1242,8 +1242,6 @@ static int mt7921_suspend(struct ieee80211_hw *hw,
> >> >	mt7921_mutex_acquire(dev);
> >> >
> >> >	clear_bit(MT76_STATE_RUNNING, &phy->mt76->state);
> >> > -
> >> > -	set_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
> >> >	ieee80211_iterate_active_interfaces(hw,
> >> >					    IEEE80211_IFACE_ITER_RESUME_ALL,
> >> >					    mt76_connac_mcu_set_suspend_iter, @@ -1262,7 +1260,6 @@
> >> > static int mt7921_resume(struct ieee80211_hw *hw)
> >> >	mt7921_mutex_acquire(dev);
> >> >
> >> >	set_bit(MT76_STATE_RUNNING, &phy->mt76->state);
> >> > -	clear_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
> >> >	ieee80211_iterate_active_interfaces(hw,
> >> >					    IEEE80211_IFACE_ITER_RESUME_ALL,
> >> >					    mt76_connac_mcu_set_suspend_iter, diff --git
> >> > a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> >> > b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> >> > index 5fee489c7a99..5c88b6b8d097 100644
> >> > --- a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> >> > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> >> > @@ -206,6 +206,8 @@ static int mt7921s_suspend(struct device *__dev)
> >> >	int err;
> >> >
> >> >	pm->suspended = true;
> >> > +	set_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
> >> > +
> >> >	cancel_delayed_work_sync(&pm->ps_work);
> >> >	cancel_work_sync(&pm->wake_work);
> >> >
> >> > @@ -213,10 +215,6 @@ static int mt7921s_suspend(struct device *__dev)
> >> >	if (err < 0)
> >> >		goto restore_suspend;
> >> >
> >> > -	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
> >> > -	if (err)
> >> > -		goto restore_suspend;
> >> > -
> >> >	/* always enable deep sleep during suspend to reduce
> >> >	 * power consumption
> >> >	 */
> >> > @@ -224,34 +222,45 @@ static int mt7921s_suspend(struct device
> >> > *__dev)
> >> >
> >> >	mt76_txq_schedule_all(&dev->mphy);
> >> >	mt76_worker_disable(&mdev->tx_worker);
> >> > -	mt76_worker_disable(&mdev->sdio.txrx_worker);
> >> >	mt76_worker_disable(&mdev->sdio.status_worker);
> >> > -	mt76_worker_disable(&mdev->sdio.net_worker);
> >> >	cancel_work_sync(&mdev->sdio.stat_work);
> >> >	clear_bit(MT76_READING_STATS, &dev->mphy.state);
> >> > -
> >> >	mt76_tx_status_check(mdev, true);
> >> >
> >> > -	err = mt7921_mcu_fw_pmctrl(dev);
> >> > +	mt76_worker_schedule(&mdev->sdio.txrx_worker);
> >>
> >> I guess mt76_worker_disable() is supposed to do it, right?
> >> I guess we can assume txrx_worker is no longer running here, right?
> >
> >I can see it now, txrx_worker can be running on the different cpu.
> >I guess we need to add just the wait_event_timeout() and move
> >mt76_connac_mcu_set_hif_suspend() below. What do you think?
> >Can you please try the chunk below?
> 
> mt76_connac_mcu_set_hif_suspend have to rely on txrx_worker and net_worker to
> send the command MCU_UNI_CMD_HIF_CTRL and receive the corresponding event,
> so that is why we cannnot disable txrx_worker and net_worker with mt76_worker_disable
> until the mt76_connac_mcu_set_hif_suspend is done.

ack, right. I forgot about this double dependency :)

> 
> >
> >Regards,
> >Lorenzo
> >
> >>
> >> > +	wait_event_timeout(dev->mt76.sdio.wait,
> >> > +			   mt76s_txqs_empty(&dev->mt76), 5 * HZ);
> >> > +
> >> > +	/* It is supposed that SDIO bus is idle at the point */
> >> > +	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
> >> >	if (err)
> >> >		goto restore_worker;
> >> >
> >> > +	mt76_worker_disable(&mdev->sdio.txrx_worker);
> >> > +	mt76_worker_disable(&mdev->sdio.net_worker);
> >> > +
> >> > +	err = mt7921_mcu_fw_pmctrl(dev);
> >> > +	if (err)
> >> > +		goto restore_txrx_worker;
> >> > +
> >> >	sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
> >> >
> >> >	return 0;
> >> >
> >> > +restore_txrx_worker:
> >> > +	mt76_worker_enable(&mdev->sdio.net_worker);
> >> > +	mt76_worker_enable(&mdev->sdio.txrx_worker);
> >> > +	mt76_connac_mcu_set_hif_suspend(mdev, false);
> >> > +
> >> >  restore_worker:
> >> >	mt76_worker_enable(&mdev->tx_worker);
> >> > -	mt76_worker_enable(&mdev->sdio.txrx_worker);
> >> >	mt76_worker_enable(&mdev->sdio.status_worker);
> >> > -	mt76_worker_enable(&mdev->sdio.net_worker);
> >> >
> >> >	if (!pm->ds_enable)
> >> >		mt76_connac_mcu_set_deep_sleep(mdev, false);
> >> >
> >> > -	mt76_connac_mcu_set_hif_suspend(mdev, false);
> >> > -
> >> >  restore_suspend:
> >> > +	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
> >> >	pm->suspended = false;
> >> >
> >> >	return err;
> >> > @@ -266,6 +275,7 @@ static int mt7921s_resume(struct device *__dev)
> >> >	int err;
> >> >
> >> >	pm->suspended = false;
> >> > +	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
> >> >
> >> >	err = mt7921_mcu_drv_pmctrl(dev);
> >> >	if (err < 0)
> >> > diff --git a/drivers/net/wireless/mediatek/mt76/sdio.c
> >> > b/drivers/net/wireless/mediatek/mt76/sdio.c
> >> > index c99acc21225e..b0bc7be0fb1f 100644
> >> > --- a/drivers/net/wireless/mediatek/mt76/sdio.c
> >> > +++ b/drivers/net/wireless/mediatek/mt76/sdio.c
> >> > @@ -479,7 +479,8 @@ static void mt76s_status_worker(struct mt76_worker *w)
> >> >			resched = true;
> >> >
> >> >		if (dev->drv->tx_status_data &&
> >> > -		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state))
> >> > +		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state) &&
> >> > +		    !test_bit(MT76_STATE_SUSPEND, &dev->phy.state))
> >> >			queue_work(dev->wq, &dev->sdio.stat_work);
> >> >	} while (nframes > 0);
> >> >
> >> > diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> >> > b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> >> > index 649a56790b89..801590a0a334 100644
> >> > --- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> >> > +++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> >> > @@ -317,7 +317,8 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
> >> >		if (ret > 0)
> >> >			nframes += ret;
> >> >
> >> > -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
> >> > +		if (test_bit(MT76_MCU_RESET, &dev->phy.state) ||
> >> > +		    test_bit(MT76_STATE_SUSPEND, &dev->phy.state)) {
> >> >			if (!mt76s_txqs_empty(dev))
> >> >				continue;
> >> >			else
> >>
> >> since mt76s_tx_run_queue will not run if MT76_MCU_RESET is set, do we
> >> really need to check it for each iteration or is fine something like:
> >>
> >> diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> >> b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> >> index 649a56790b89..68f30a83fa5d 100644
> >> --- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> >> +++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> >> @@ -317,14 +317,12 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
> >>		if (ret > 0)
> >>			nframes += ret;
> >>
> >> -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
> >> -			if (!mt76s_txqs_empty(dev))
> >> -				continue;
> >> -			else
> >> -				wake_up(&sdio->wait);
> >> -		}
> >>	} while (nframes > 0);
> >>
> >> +	if (test_bit(MT76_MCU_RESET, &dev->phy.state) &&
> >> +	    mt76s_txqs_empty(dev))
> >> +		wake_up(&sdio->wait);
> >> +
> 
> If doing so, mt76s_txqs_empty may not always be true because enqueuing packets
> to q_tx or MCU command to q_mcu simultanenously from the other contexts in
> different cpu is possible.
> 
> It seemed to me we should check it for each iteration to guarantee that we can
> wake up the one that is waiting for the all the queues are empty at some time.

IIUC what we are interested here is there are no queued frames into the hw
queues during suspend or reset, right?

> 
> >>	/* enable interrupt */
> >>	sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
> >>	sdio_release_host(sdio->func);
> >>
> >> Regards,
> >> Lorenzo
> >>
> >> > --
> >> > 2.25.1
> >> >
> >
> >
> >
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend

[Lorenzo Bianconi]

[-- Attachment #1.1: Type: text/plain, Size: 10067 bytes --]

> From: Sean Wang <sean.wang@mediatek.com>
> 
> >> > From: Sean Wang <sean.wang@mediatek.com>
> >> >
> >> > According to the MT7921S firmware, the cmd MCU_UNI_CMD_HIF_CTRL have
> >> > to be last MCU command to execute in suspend handler and all data
> >> > traffic have to be stopped before the cmd MCU_UNI_CMD_HIF_CTRL
> >> > starts as well in order that mt7921 can successfully fall into the deep sleep mode.
> >> >
> >> > Where we reuse the flag MT76_STATE_SUSPEND and avoid creating
> >> > another global flag to stop all of the traffic onto the SDIO bus.
> >> >
> >> > Fixes: 48fab5bbef40 ("mt76: mt7921: introduce mt7921s support")
> >> > Reported-by: Leon Yen <leon.yen@mediatek.com>
> >> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> >> > ---
> >> >  .../wireless/mediatek/mt76/mt76_connac_mcu.c  |  2 +-
> >> > .../net/wireless/mediatek/mt76/mt7921/main.c  |  3 --
> >> > .../net/wireless/mediatek/mt76/mt7921/sdio.c  | 34 ++++++++++++-------
> >> >  drivers/net/wireless/mediatek/mt76/sdio.c     |  3 +-
> >> >  .../net/wireless/mediatek/mt76/sdio_txrx.c    |  3 +-
> >> >  5 files changed, 27 insertions(+), 18 deletions(-)
> >> >
> >> > diff --git a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> >> > b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> >> > index 26b4b875dcc0..61c4c86e79c8 100644
> >> > --- a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> >> > +++ b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> >> > @@ -2461,7 +2461,7 @@ void mt76_connac_mcu_set_suspend_iter(void *priv, u8 *mac,
> >> >				      struct ieee80211_vif *vif)  {
> >> >	struct mt76_phy *phy = priv;
> >> > -	bool suspend = test_bit(MT76_STATE_SUSPEND, &phy->state);
> >> > +	bool suspend = !test_bit(MT76_STATE_RUNNING, &phy->state);
> >> >	struct ieee80211_hw *hw = phy->hw;
> >> >	struct cfg80211_wowlan *wowlan = hw->wiphy->wowlan_config;
> >> >	int i;
> >> > diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> >> > b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> >> > index e022251b4006..0b2a6b7f22ea 100644
> >> > --- a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> >> > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> >> > @@ -1242,8 +1242,6 @@ static int mt7921_suspend(struct ieee80211_hw *hw,
> >> >	mt7921_mutex_acquire(dev);
> >> >
> >> >	clear_bit(MT76_STATE_RUNNING, &phy->mt76->state);
> >> > -
> >> > -	set_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
> >> >	ieee80211_iterate_active_interfaces(hw,
> >> >					    IEEE80211_IFACE_ITER_RESUME_ALL,
> >> >					    mt76_connac_mcu_set_suspend_iter, @@ -1262,7 +1260,6 @@
> >> > static int mt7921_resume(struct ieee80211_hw *hw)
> >> >	mt7921_mutex_acquire(dev);
> >> >
> >> >	set_bit(MT76_STATE_RUNNING, &phy->mt76->state);
> >> > -	clear_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
> >> >	ieee80211_iterate_active_interfaces(hw,
> >> >					    IEEE80211_IFACE_ITER_RESUME_ALL,
> >> >					    mt76_connac_mcu_set_suspend_iter, diff --git
> >> > a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> >> > b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> >> > index 5fee489c7a99..5c88b6b8d097 100644
> >> > --- a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> >> > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> >> > @@ -206,6 +206,8 @@ static int mt7921s_suspend(struct device *__dev)
> >> >	int err;
> >> >
> >> >	pm->suspended = true;
> >> > +	set_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
> >> > +
> >> >	cancel_delayed_work_sync(&pm->ps_work);
> >> >	cancel_work_sync(&pm->wake_work);
> >> >
> >> > @@ -213,10 +215,6 @@ static int mt7921s_suspend(struct device *__dev)
> >> >	if (err < 0)
> >> >		goto restore_suspend;
> >> >
> >> > -	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
> >> > -	if (err)
> >> > -		goto restore_suspend;
> >> > -
> >> >	/* always enable deep sleep during suspend to reduce
> >> >	 * power consumption
> >> >	 */
> >> > @@ -224,34 +222,45 @@ static int mt7921s_suspend(struct device
> >> > *__dev)
> >> >
> >> >	mt76_txq_schedule_all(&dev->mphy);
> >> >	mt76_worker_disable(&mdev->tx_worker);
> >> > -	mt76_worker_disable(&mdev->sdio.txrx_worker);
> >> >	mt76_worker_disable(&mdev->sdio.status_worker);
> >> > -	mt76_worker_disable(&mdev->sdio.net_worker);
> >> >	cancel_work_sync(&mdev->sdio.stat_work);
> >> >	clear_bit(MT76_READING_STATS, &dev->mphy.state);
> >> > -
> >> >	mt76_tx_status_check(mdev, true);
> >> >
> >> > -	err = mt7921_mcu_fw_pmctrl(dev);
> >> > +	mt76_worker_schedule(&mdev->sdio.txrx_worker);
> >>
> >> I guess mt76_worker_disable() is supposed to do it, right?
> >> I guess we can assume txrx_worker is no longer running here, right?
> >
> >I can see it now, txrx_worker can be running on the different cpu.
> >I guess we need to add just the wait_event_timeout() and move
> >mt76_connac_mcu_set_hif_suspend() below. What do you think?
> >Can you please try the chunk below?
> 
> mt76_connac_mcu_set_hif_suspend have to rely on txrx_worker and net_worker to
> send the command MCU_UNI_CMD_HIF_CTRL and receive the corresponding event,
> so that is why we cannnot disable txrx_worker and net_worker with mt76_worker_disable
> until the mt76_connac_mcu_set_hif_suspend is done.

ack, right. I forgot about this double dependency :)

> 
> >
> >Regards,
> >Lorenzo
> >
> >>
> >> > +	wait_event_timeout(dev->mt76.sdio.wait,
> >> > +			   mt76s_txqs_empty(&dev->mt76), 5 * HZ);
> >> > +
> >> > +	/* It is supposed that SDIO bus is idle at the point */
> >> > +	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
> >> >	if (err)
> >> >		goto restore_worker;
> >> >
> >> > +	mt76_worker_disable(&mdev->sdio.txrx_worker);
> >> > +	mt76_worker_disable(&mdev->sdio.net_worker);
> >> > +
> >> > +	err = mt7921_mcu_fw_pmctrl(dev);
> >> > +	if (err)
> >> > +		goto restore_txrx_worker;
> >> > +
> >> >	sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
> >> >
> >> >	return 0;
> >> >
> >> > +restore_txrx_worker:
> >> > +	mt76_worker_enable(&mdev->sdio.net_worker);
> >> > +	mt76_worker_enable(&mdev->sdio.txrx_worker);
> >> > +	mt76_connac_mcu_set_hif_suspend(mdev, false);
> >> > +
> >> >  restore_worker:
> >> >	mt76_worker_enable(&mdev->tx_worker);
> >> > -	mt76_worker_enable(&mdev->sdio.txrx_worker);
> >> >	mt76_worker_enable(&mdev->sdio.status_worker);
> >> > -	mt76_worker_enable(&mdev->sdio.net_worker);
> >> >
> >> >	if (!pm->ds_enable)
> >> >		mt76_connac_mcu_set_deep_sleep(mdev, false);
> >> >
> >> > -	mt76_connac_mcu_set_hif_suspend(mdev, false);
> >> > -
> >> >  restore_suspend:
> >> > +	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
> >> >	pm->suspended = false;
> >> >
> >> >	return err;
> >> > @@ -266,6 +275,7 @@ static int mt7921s_resume(struct device *__dev)
> >> >	int err;
> >> >
> >> >	pm->suspended = false;
> >> > +	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
> >> >
> >> >	err = mt7921_mcu_drv_pmctrl(dev);
> >> >	if (err < 0)
> >> > diff --git a/drivers/net/wireless/mediatek/mt76/sdio.c
> >> > b/drivers/net/wireless/mediatek/mt76/sdio.c
> >> > index c99acc21225e..b0bc7be0fb1f 100644
> >> > --- a/drivers/net/wireless/mediatek/mt76/sdio.c
> >> > +++ b/drivers/net/wireless/mediatek/mt76/sdio.c
> >> > @@ -479,7 +479,8 @@ static void mt76s_status_worker(struct mt76_worker *w)
> >> >			resched = true;
> >> >
> >> >		if (dev->drv->tx_status_data &&
> >> > -		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state))
> >> > +		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state) &&
> >> > +		    !test_bit(MT76_STATE_SUSPEND, &dev->phy.state))
> >> >			queue_work(dev->wq, &dev->sdio.stat_work);
> >> >	} while (nframes > 0);
> >> >
> >> > diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> >> > b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> >> > index 649a56790b89..801590a0a334 100644
> >> > --- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> >> > +++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> >> > @@ -317,7 +317,8 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
> >> >		if (ret > 0)
> >> >			nframes += ret;
> >> >
> >> > -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
> >> > +		if (test_bit(MT76_MCU_RESET, &dev->phy.state) ||
> >> > +		    test_bit(MT76_STATE_SUSPEND, &dev->phy.state)) {
> >> >			if (!mt76s_txqs_empty(dev))
> >> >				continue;
> >> >			else
> >>
> >> since mt76s_tx_run_queue will not run if MT76_MCU_RESET is set, do we
> >> really need to check it for each iteration or is fine something like:
> >>
> >> diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> >> b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> >> index 649a56790b89..68f30a83fa5d 100644
> >> --- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> >> +++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> >> @@ -317,14 +317,12 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
> >>		if (ret > 0)
> >>			nframes += ret;
> >>
> >> -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
> >> -			if (!mt76s_txqs_empty(dev))
> >> -				continue;
> >> -			else
> >> -				wake_up(&sdio->wait);
> >> -		}
> >>	} while (nframes > 0);
> >>
> >> +	if (test_bit(MT76_MCU_RESET, &dev->phy.state) &&
> >> +	    mt76s_txqs_empty(dev))
> >> +		wake_up(&sdio->wait);
> >> +
> 
> If doing so, mt76s_txqs_empty may not always be true because enqueuing packets
> to q_tx or MCU command to q_mcu simultanenously from the other contexts in
> different cpu is possible.
> 
> It seemed to me we should check it for each iteration to guarantee that we can
> wake up the one that is waiting for the all the queues are empty at some time.

IIUC what we are interested here is there are no queued frames into the hw
queues during suspend or reset, right?

> 
> >>	/* enable interrupt */
> >>	sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
> >>	sdio_release_host(sdio->func);
> >>
> >> Regards,
> >> Lorenzo
> >>
> >> > --
> >> > 2.25.1
> >> >
> >
> >
> >
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

>> > From: Sean Wang <sean.wang@mediatek.com>
>> >
>> > According to the MT7921S firmware, the cmd MCU_UNI_CMD_HIF_CTRL have
>> > to be last MCU command to execute in suspend handler and all data
>> > traffic have to be stopped before the cmd MCU_UNI_CMD_HIF_CTRL
>> > starts as well in order that mt7921 can successfully fall into the deep sleep mode.
>> >
>> > Where we reuse the flag MT76_STATE_SUSPEND and avoid creating
>> > another global flag to stop all of the traffic onto the SDIO bus.
>> >
>> > Fixes: 48fab5bbef40 ("mt76: mt7921: introduce mt7921s support")
>> > Reported-by: Leon Yen <leon.yen@mediatek.com>
>> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
>> > ---
>> >  .../wireless/mediatek/mt76/mt76_connac_mcu.c  |  2 +-
>> > .../net/wireless/mediatek/mt76/mt7921/main.c  |  3 --
>> > .../net/wireless/mediatek/mt76/mt7921/sdio.c  | 34 ++++++++++++-------
>> >  drivers/net/wireless/mediatek/mt76/sdio.c     |  3 +-
>> >  .../net/wireless/mediatek/mt76/sdio_txrx.c    |  3 +-
>> >  5 files changed, 27 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
>> > b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
>> > index 26b4b875dcc0..61c4c86e79c8 100644
>> > --- a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
>> > +++ b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
>> > @@ -2461,7 +2461,7 @@ void mt76_connac_mcu_set_suspend_iter(void *priv, u8 *mac,
>> >				      struct ieee80211_vif *vif)  {
>> >	struct mt76_phy *phy = priv;
>> > -	bool suspend = test_bit(MT76_STATE_SUSPEND, &phy->state);
>> > +	bool suspend = !test_bit(MT76_STATE_RUNNING, &phy->state);
>> >	struct ieee80211_hw *hw = phy->hw;
>> >	struct cfg80211_wowlan *wowlan = hw->wiphy->wowlan_config;
>> >	int i;
>> > diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
>> > b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
>> > index e022251b4006..0b2a6b7f22ea 100644
>> > --- a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
>> > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
>> > @@ -1242,8 +1242,6 @@ static int mt7921_suspend(struct ieee80211_hw *hw,
>> >	mt7921_mutex_acquire(dev);
>> >
>> >	clear_bit(MT76_STATE_RUNNING, &phy->mt76->state);
>> > -
>> > -	set_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
>> >	ieee80211_iterate_active_interfaces(hw,
>> >					    IEEE80211_IFACE_ITER_RESUME_ALL,
>> >					    mt76_connac_mcu_set_suspend_iter, @@ -1262,7 +1260,6 @@
>> > static int mt7921_resume(struct ieee80211_hw *hw)
>> >	mt7921_mutex_acquire(dev);
>> >
>> >	set_bit(MT76_STATE_RUNNING, &phy->mt76->state);
>> > -	clear_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
>> >	ieee80211_iterate_active_interfaces(hw,
>> >					    IEEE80211_IFACE_ITER_RESUME_ALL,
>> >					    mt76_connac_mcu_set_suspend_iter, diff --git
>> > a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
>> > b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
>> > index 5fee489c7a99..5c88b6b8d097 100644
>> > --- a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
>> > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
>> > @@ -206,6 +206,8 @@ static int mt7921s_suspend(struct device *__dev)
>> >	int err;
>> >
>> >	pm->suspended = true;
>> > +	set_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
>> > +
>> >	cancel_delayed_work_sync(&pm->ps_work);
>> >	cancel_work_sync(&pm->wake_work);
>> >
>> > @@ -213,10 +215,6 @@ static int mt7921s_suspend(struct device *__dev)
>> >	if (err < 0)
>> >		goto restore_suspend;
>> >
>> > -	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
>> > -	if (err)
>> > -		goto restore_suspend;
>> > -
>> >	/* always enable deep sleep during suspend to reduce
>> >	 * power consumption
>> >	 */
>> > @@ -224,34 +222,45 @@ static int mt7921s_suspend(struct device
>> > *__dev)
>> >
>> >	mt76_txq_schedule_all(&dev->mphy);
>> >	mt76_worker_disable(&mdev->tx_worker);
>> > -	mt76_worker_disable(&mdev->sdio.txrx_worker);
>> >	mt76_worker_disable(&mdev->sdio.status_worker);
>> > -	mt76_worker_disable(&mdev->sdio.net_worker);
>> >	cancel_work_sync(&mdev->sdio.stat_work);
>> >	clear_bit(MT76_READING_STATS, &dev->mphy.state);
>> > -
>> >	mt76_tx_status_check(mdev, true);
>> >
>> > -	err = mt7921_mcu_fw_pmctrl(dev);
>> > +	mt76_worker_schedule(&mdev->sdio.txrx_worker);
>>
>> I guess mt76_worker_disable() is supposed to do it, right?
>> I guess we can assume txrx_worker is no longer running here, right?
>
>I can see it now, txrx_worker can be running on the different cpu.
>I guess we need to add just the wait_event_timeout() and move
>mt76_connac_mcu_set_hif_suspend() below. What do you think?
>Can you please try the chunk below?

mt76_connac_mcu_set_hif_suspend have to rely on txrx_worker and net_worker to
send the command MCU_UNI_CMD_HIF_CTRL and receive the corresponding event,
so that is why we cannnot disable txrx_worker and net_worker with mt76_worker_disable
until the mt76_connac_mcu_set_hif_suspend is done.

>
>Regards,
>Lorenzo
>
>>
>> > +	wait_event_timeout(dev->mt76.sdio.wait,
>> > +			   mt76s_txqs_empty(&dev->mt76), 5 * HZ);
>> > +
>> > +	/* It is supposed that SDIO bus is idle at the point */
>> > +	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
>> >	if (err)
>> >		goto restore_worker;
>> >
>> > +	mt76_worker_disable(&mdev->sdio.txrx_worker);
>> > +	mt76_worker_disable(&mdev->sdio.net_worker);
>> > +
>> > +	err = mt7921_mcu_fw_pmctrl(dev);
>> > +	if (err)
>> > +		goto restore_txrx_worker;
>> > +
>> >	sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
>> >
>> >	return 0;
>> >
>> > +restore_txrx_worker:
>> > +	mt76_worker_enable(&mdev->sdio.net_worker);
>> > +	mt76_worker_enable(&mdev->sdio.txrx_worker);
>> > +	mt76_connac_mcu_set_hif_suspend(mdev, false);
>> > +
>> >  restore_worker:
>> >	mt76_worker_enable(&mdev->tx_worker);
>> > -	mt76_worker_enable(&mdev->sdio.txrx_worker);
>> >	mt76_worker_enable(&mdev->sdio.status_worker);
>> > -	mt76_worker_enable(&mdev->sdio.net_worker);
>> >
>> >	if (!pm->ds_enable)
>> >		mt76_connac_mcu_set_deep_sleep(mdev, false);
>> >
>> > -	mt76_connac_mcu_set_hif_suspend(mdev, false);
>> > -
>> >  restore_suspend:
>> > +	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
>> >	pm->suspended = false;
>> >
>> >	return err;
>> > @@ -266,6 +275,7 @@ static int mt7921s_resume(struct device *__dev)
>> >	int err;
>> >
>> >	pm->suspended = false;
>> > +	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
>> >
>> >	err = mt7921_mcu_drv_pmctrl(dev);
>> >	if (err < 0)
>> > diff --git a/drivers/net/wireless/mediatek/mt76/sdio.c
>> > b/drivers/net/wireless/mediatek/mt76/sdio.c
>> > index c99acc21225e..b0bc7be0fb1f 100644
>> > --- a/drivers/net/wireless/mediatek/mt76/sdio.c
>> > +++ b/drivers/net/wireless/mediatek/mt76/sdio.c
>> > @@ -479,7 +479,8 @@ static void mt76s_status_worker(struct mt76_worker *w)
>> >			resched = true;
>> >
>> >		if (dev->drv->tx_status_data &&
>> > -		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state))
>> > +		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state) &&
>> > +		    !test_bit(MT76_STATE_SUSPEND, &dev->phy.state))
>> >			queue_work(dev->wq, &dev->sdio.stat_work);
>> >	} while (nframes > 0);
>> >
>> > diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
>> > b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
>> > index 649a56790b89..801590a0a334 100644
>> > --- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
>> > +++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
>> > @@ -317,7 +317,8 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
>> >		if (ret > 0)
>> >			nframes += ret;
>> >
>> > -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
>> > +		if (test_bit(MT76_MCU_RESET, &dev->phy.state) ||
>> > +		    test_bit(MT76_STATE_SUSPEND, &dev->phy.state)) {
>> >			if (!mt76s_txqs_empty(dev))
>> >				continue;
>> >			else
>>
>> since mt76s_tx_run_queue will not run if MT76_MCU_RESET is set, do we
>> really need to check it for each iteration or is fine something like:
>>
>> diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
>> b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
>> index 649a56790b89..68f30a83fa5d 100644
>> --- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
>> +++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
>> @@ -317,14 +317,12 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
>>		if (ret > 0)
>>			nframes += ret;
>>
>> -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
>> -			if (!mt76s_txqs_empty(dev))
>> -				continue;
>> -			else
>> -				wake_up(&sdio->wait);
>> -		}
>>	} while (nframes > 0);
>>
>> +	if (test_bit(MT76_MCU_RESET, &dev->phy.state) &&
>> +	    mt76s_txqs_empty(dev))
>> +		wake_up(&sdio->wait);
>> +

If doing so, mt76s_txqs_empty may not always be true because enqueuing packets
to q_tx or MCU command to q_mcu simultanenously from the other contexts in
different cpu is possible.

It seemed to me we should check it for each iteration to guarantee that we can
wake up the one that is waiting for the all the queues are empty at some time.

>>	/* enable interrupt */
>>	sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
>>	sdio_release_host(sdio->func);
>>
>> Regards,
>> Lorenzo
>>
>> > --
>> > 2.25.1
>> >
>
>
>

Re: [PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

>> > From: Sean Wang <sean.wang@mediatek.com>
>> >
>> > According to the MT7921S firmware, the cmd MCU_UNI_CMD_HIF_CTRL have
>> > to be last MCU command to execute in suspend handler and all data
>> > traffic have to be stopped before the cmd MCU_UNI_CMD_HIF_CTRL
>> > starts as well in order that mt7921 can successfully fall into the deep sleep mode.
>> >
>> > Where we reuse the flag MT76_STATE_SUSPEND and avoid creating
>> > another global flag to stop all of the traffic onto the SDIO bus.
>> >
>> > Fixes: 48fab5bbef40 ("mt76: mt7921: introduce mt7921s support")
>> > Reported-by: Leon Yen <leon.yen@mediatek.com>
>> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
>> > ---
>> >  .../wireless/mediatek/mt76/mt76_connac_mcu.c  |  2 +-
>> > .../net/wireless/mediatek/mt76/mt7921/main.c  |  3 --
>> > .../net/wireless/mediatek/mt76/mt7921/sdio.c  | 34 ++++++++++++-------
>> >  drivers/net/wireless/mediatek/mt76/sdio.c     |  3 +-
>> >  .../net/wireless/mediatek/mt76/sdio_txrx.c    |  3 +-
>> >  5 files changed, 27 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
>> > b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
>> > index 26b4b875dcc0..61c4c86e79c8 100644
>> > --- a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
>> > +++ b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
>> > @@ -2461,7 +2461,7 @@ void mt76_connac_mcu_set_suspend_iter(void *priv, u8 *mac,
>> >				      struct ieee80211_vif *vif)  {
>> >	struct mt76_phy *phy = priv;
>> > -	bool suspend = test_bit(MT76_STATE_SUSPEND, &phy->state);
>> > +	bool suspend = !test_bit(MT76_STATE_RUNNING, &phy->state);
>> >	struct ieee80211_hw *hw = phy->hw;
>> >	struct cfg80211_wowlan *wowlan = hw->wiphy->wowlan_config;
>> >	int i;
>> > diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
>> > b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
>> > index e022251b4006..0b2a6b7f22ea 100644
>> > --- a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
>> > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
>> > @@ -1242,8 +1242,6 @@ static int mt7921_suspend(struct ieee80211_hw *hw,
>> >	mt7921_mutex_acquire(dev);
>> >
>> >	clear_bit(MT76_STATE_RUNNING, &phy->mt76->state);
>> > -
>> > -	set_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
>> >	ieee80211_iterate_active_interfaces(hw,
>> >					    IEEE80211_IFACE_ITER_RESUME_ALL,
>> >					    mt76_connac_mcu_set_suspend_iter, @@ -1262,7 +1260,6 @@
>> > static int mt7921_resume(struct ieee80211_hw *hw)
>> >	mt7921_mutex_acquire(dev);
>> >
>> >	set_bit(MT76_STATE_RUNNING, &phy->mt76->state);
>> > -	clear_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
>> >	ieee80211_iterate_active_interfaces(hw,
>> >					    IEEE80211_IFACE_ITER_RESUME_ALL,
>> >					    mt76_connac_mcu_set_suspend_iter, diff --git
>> > a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
>> > b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
>> > index 5fee489c7a99..5c88b6b8d097 100644
>> > --- a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
>> > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
>> > @@ -206,6 +206,8 @@ static int mt7921s_suspend(struct device *__dev)
>> >	int err;
>> >
>> >	pm->suspended = true;
>> > +	set_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
>> > +
>> >	cancel_delayed_work_sync(&pm->ps_work);
>> >	cancel_work_sync(&pm->wake_work);
>> >
>> > @@ -213,10 +215,6 @@ static int mt7921s_suspend(struct device *__dev)
>> >	if (err < 0)
>> >		goto restore_suspend;
>> >
>> > -	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
>> > -	if (err)
>> > -		goto restore_suspend;
>> > -
>> >	/* always enable deep sleep during suspend to reduce
>> >	 * power consumption
>> >	 */
>> > @@ -224,34 +222,45 @@ static int mt7921s_suspend(struct device
>> > *__dev)
>> >
>> >	mt76_txq_schedule_all(&dev->mphy);
>> >	mt76_worker_disable(&mdev->tx_worker);
>> > -	mt76_worker_disable(&mdev->sdio.txrx_worker);
>> >	mt76_worker_disable(&mdev->sdio.status_worker);
>> > -	mt76_worker_disable(&mdev->sdio.net_worker);
>> >	cancel_work_sync(&mdev->sdio.stat_work);
>> >	clear_bit(MT76_READING_STATS, &dev->mphy.state);
>> > -
>> >	mt76_tx_status_check(mdev, true);
>> >
>> > -	err = mt7921_mcu_fw_pmctrl(dev);
>> > +	mt76_worker_schedule(&mdev->sdio.txrx_worker);
>>
>> I guess mt76_worker_disable() is supposed to do it, right?
>> I guess we can assume txrx_worker is no longer running here, right?
>
>I can see it now, txrx_worker can be running on the different cpu.
>I guess we need to add just the wait_event_timeout() and move
>mt76_connac_mcu_set_hif_suspend() below. What do you think?
>Can you please try the chunk below?

mt76_connac_mcu_set_hif_suspend have to rely on txrx_worker and net_worker to
send the command MCU_UNI_CMD_HIF_CTRL and receive the corresponding event,
so that is why we cannnot disable txrx_worker and net_worker with mt76_worker_disable
until the mt76_connac_mcu_set_hif_suspend is done.

>
>Regards,
>Lorenzo
>
>>
>> > +	wait_event_timeout(dev->mt76.sdio.wait,
>> > +			   mt76s_txqs_empty(&dev->mt76), 5 * HZ);
>> > +
>> > +	/* It is supposed that SDIO bus is idle at the point */
>> > +	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
>> >	if (err)
>> >		goto restore_worker;
>> >
>> > +	mt76_worker_disable(&mdev->sdio.txrx_worker);
>> > +	mt76_worker_disable(&mdev->sdio.net_worker);
>> > +
>> > +	err = mt7921_mcu_fw_pmctrl(dev);
>> > +	if (err)
>> > +		goto restore_txrx_worker;
>> > +
>> >	sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
>> >
>> >	return 0;
>> >
>> > +restore_txrx_worker:
>> > +	mt76_worker_enable(&mdev->sdio.net_worker);
>> > +	mt76_worker_enable(&mdev->sdio.txrx_worker);
>> > +	mt76_connac_mcu_set_hif_suspend(mdev, false);
>> > +
>> >  restore_worker:
>> >	mt76_worker_enable(&mdev->tx_worker);
>> > -	mt76_worker_enable(&mdev->sdio.txrx_worker);
>> >	mt76_worker_enable(&mdev->sdio.status_worker);
>> > -	mt76_worker_enable(&mdev->sdio.net_worker);
>> >
>> >	if (!pm->ds_enable)
>> >		mt76_connac_mcu_set_deep_sleep(mdev, false);
>> >
>> > -	mt76_connac_mcu_set_hif_suspend(mdev, false);
>> > -
>> >  restore_suspend:
>> > +	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
>> >	pm->suspended = false;
>> >
>> >	return err;
>> > @@ -266,6 +275,7 @@ static int mt7921s_resume(struct device *__dev)
>> >	int err;
>> >
>> >	pm->suspended = false;
>> > +	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
>> >
>> >	err = mt7921_mcu_drv_pmctrl(dev);
>> >	if (err < 0)
>> > diff --git a/drivers/net/wireless/mediatek/mt76/sdio.c
>> > b/drivers/net/wireless/mediatek/mt76/sdio.c
>> > index c99acc21225e..b0bc7be0fb1f 100644
>> > --- a/drivers/net/wireless/mediatek/mt76/sdio.c
>> > +++ b/drivers/net/wireless/mediatek/mt76/sdio.c
>> > @@ -479,7 +479,8 @@ static void mt76s_status_worker(struct mt76_worker *w)
>> >			resched = true;
>> >
>> >		if (dev->drv->tx_status_data &&
>> > -		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state))
>> > +		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state) &&
>> > +		    !test_bit(MT76_STATE_SUSPEND, &dev->phy.state))
>> >			queue_work(dev->wq, &dev->sdio.stat_work);
>> >	} while (nframes > 0);
>> >
>> > diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
>> > b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
>> > index 649a56790b89..801590a0a334 100644
>> > --- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
>> > +++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
>> > @@ -317,7 +317,8 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
>> >		if (ret > 0)
>> >			nframes += ret;
>> >
>> > -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
>> > +		if (test_bit(MT76_MCU_RESET, &dev->phy.state) ||
>> > +		    test_bit(MT76_STATE_SUSPEND, &dev->phy.state)) {
>> >			if (!mt76s_txqs_empty(dev))
>> >				continue;
>> >			else
>>
>> since mt76s_tx_run_queue will not run if MT76_MCU_RESET is set, do we
>> really need to check it for each iteration or is fine something like:
>>
>> diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
>> b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
>> index 649a56790b89..68f30a83fa5d 100644
>> --- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
>> +++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
>> @@ -317,14 +317,12 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
>>		if (ret > 0)
>>			nframes += ret;
>>
>> -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
>> -			if (!mt76s_txqs_empty(dev))
>> -				continue;
>> -			else
>> -				wake_up(&sdio->wait);
>> -		}
>>	} while (nframes > 0);
>>
>> +	if (test_bit(MT76_MCU_RESET, &dev->phy.state) &&
>> +	    mt76s_txqs_empty(dev))
>> +		wake_up(&sdio->wait);
>> +

If doing so, mt76s_txqs_empty may not always be true because enqueuing packets
to q_tx or MCU command to q_mcu simultanenously from the other contexts in
different cpu is possible.

It seemed to me we should check it for each iteration to guarantee that we can
wake up the one that is waiting for the all the queues are empty at some time.

>>	/* enable interrupt */
>>	sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
>>	sdio_release_host(sdio->func);
>>
>> Regards,
>> Lorenzo
>>
>> > --
>> > 2.25.1
>> >
>
>
>

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 8628 bytes --]

> > From: Sean Wang <sean.wang@mediatek.com>
> > 
> > According to the MT7921S firmware, the cmd MCU_UNI_CMD_HIF_CTRL have to
> > be last MCU command to execute in suspend handler and all data traffic
> > have to be stopped before the cmd MCU_UNI_CMD_HIF_CTRL starts as well
> > in order that mt7921 can successfully fall into the deep sleep mode.
> > 
> > Where we reuse the flag MT76_STATE_SUSPEND and avoid creating
> > another global flag to stop all of the traffic onto the SDIO bus.
> > 
> > Fixes: 48fab5bbef40 ("mt76: mt7921: introduce mt7921s support")
> > Reported-by: Leon Yen <leon.yen@mediatek.com>
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > ---
> >  .../wireless/mediatek/mt76/mt76_connac_mcu.c  |  2 +-
> >  .../net/wireless/mediatek/mt76/mt7921/main.c  |  3 --
> >  .../net/wireless/mediatek/mt76/mt7921/sdio.c  | 34 ++++++++++++-------
> >  drivers/net/wireless/mediatek/mt76/sdio.c     |  3 +-
> >  .../net/wireless/mediatek/mt76/sdio_txrx.c    |  3 +-
> >  5 files changed, 27 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> > index 26b4b875dcc0..61c4c86e79c8 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> > @@ -2461,7 +2461,7 @@ void mt76_connac_mcu_set_suspend_iter(void *priv, u8 *mac,
> >  				      struct ieee80211_vif *vif)
> >  {
> >  	struct mt76_phy *phy = priv;
> > -	bool suspend = test_bit(MT76_STATE_SUSPEND, &phy->state);
> > +	bool suspend = !test_bit(MT76_STATE_RUNNING, &phy->state);
> >  	struct ieee80211_hw *hw = phy->hw;
> >  	struct cfg80211_wowlan *wowlan = hw->wiphy->wowlan_config;
> >  	int i;
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/main.c b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> > index e022251b4006..0b2a6b7f22ea 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> > @@ -1242,8 +1242,6 @@ static int mt7921_suspend(struct ieee80211_hw *hw,
> >  	mt7921_mutex_acquire(dev);
> >  
> >  	clear_bit(MT76_STATE_RUNNING, &phy->mt76->state);
> > -
> > -	set_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
> >  	ieee80211_iterate_active_interfaces(hw,
> >  					    IEEE80211_IFACE_ITER_RESUME_ALL,
> >  					    mt76_connac_mcu_set_suspend_iter,
> > @@ -1262,7 +1260,6 @@ static int mt7921_resume(struct ieee80211_hw *hw)
> >  	mt7921_mutex_acquire(dev);
> >  
> >  	set_bit(MT76_STATE_RUNNING, &phy->mt76->state);
> > -	clear_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
> >  	ieee80211_iterate_active_interfaces(hw,
> >  					    IEEE80211_IFACE_ITER_RESUME_ALL,
> >  					    mt76_connac_mcu_set_suspend_iter,
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> > index 5fee489c7a99..5c88b6b8d097 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> > @@ -206,6 +206,8 @@ static int mt7921s_suspend(struct device *__dev)
> >  	int err;
> >  
> >  	pm->suspended = true;
> > +	set_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
> > +
> >  	cancel_delayed_work_sync(&pm->ps_work);
> >  	cancel_work_sync(&pm->wake_work);
> >  
> > @@ -213,10 +215,6 @@ static int mt7921s_suspend(struct device *__dev)
> >  	if (err < 0)
> >  		goto restore_suspend;
> >  
> > -	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
> > -	if (err)
> > -		goto restore_suspend;
> > -
> >  	/* always enable deep sleep during suspend to reduce
> >  	 * power consumption
> >  	 */
> > @@ -224,34 +222,45 @@ static int mt7921s_suspend(struct device *__dev)
> >  
> >  	mt76_txq_schedule_all(&dev->mphy);
> >  	mt76_worker_disable(&mdev->tx_worker);
> > -	mt76_worker_disable(&mdev->sdio.txrx_worker);
> >  	mt76_worker_disable(&mdev->sdio.status_worker);
> > -	mt76_worker_disable(&mdev->sdio.net_worker);
> >  	cancel_work_sync(&mdev->sdio.stat_work);
> >  	clear_bit(MT76_READING_STATS, &dev->mphy.state);
> > -
> >  	mt76_tx_status_check(mdev, true);
> >  
> > -	err = mt7921_mcu_fw_pmctrl(dev);
> > +	mt76_worker_schedule(&mdev->sdio.txrx_worker);
> 
> I guess mt76_worker_disable() is supposed to do it, right?
> I guess we can assume txrx_worker is no longer running here, right?

I can see it now, txrx_worker can be running on the different cpu.
I guess we need to add just the wait_event_timeout() and move
mt76_connac_mcu_set_hif_suspend() below. What do you think?
Can you please try the chunk below?

Regards,
Lorenzo

> 
> > +	wait_event_timeout(dev->mt76.sdio.wait,
> > +			   mt76s_txqs_empty(&dev->mt76), 5 * HZ);
> > +
> > +	/* It is supposed that SDIO bus is idle at the point */
> > +	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
> >  	if (err)
> >  		goto restore_worker;
> >  
> > +	mt76_worker_disable(&mdev->sdio.txrx_worker);
> > +	mt76_worker_disable(&mdev->sdio.net_worker);
> > +
> > +	err = mt7921_mcu_fw_pmctrl(dev);
> > +	if (err)
> > +		goto restore_txrx_worker;
> > +
> >  	sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
> >  
> >  	return 0;
> >  
> > +restore_txrx_worker:
> > +	mt76_worker_enable(&mdev->sdio.net_worker);
> > +	mt76_worker_enable(&mdev->sdio.txrx_worker);
> > +	mt76_connac_mcu_set_hif_suspend(mdev, false);
> > +
> >  restore_worker:
> >  	mt76_worker_enable(&mdev->tx_worker);
> > -	mt76_worker_enable(&mdev->sdio.txrx_worker);
> >  	mt76_worker_enable(&mdev->sdio.status_worker);
> > -	mt76_worker_enable(&mdev->sdio.net_worker);
> >  
> >  	if (!pm->ds_enable)
> >  		mt76_connac_mcu_set_deep_sleep(mdev, false);
> >  
> > -	mt76_connac_mcu_set_hif_suspend(mdev, false);
> > -
> >  restore_suspend:
> > +	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
> >  	pm->suspended = false;
> >  
> >  	return err;
> > @@ -266,6 +275,7 @@ static int mt7921s_resume(struct device *__dev)
> >  	int err;
> >  
> >  	pm->suspended = false;
> > +	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
> >  
> >  	err = mt7921_mcu_drv_pmctrl(dev);
> >  	if (err < 0)
> > diff --git a/drivers/net/wireless/mediatek/mt76/sdio.c b/drivers/net/wireless/mediatek/mt76/sdio.c
> > index c99acc21225e..b0bc7be0fb1f 100644
> > --- a/drivers/net/wireless/mediatek/mt76/sdio.c
> > +++ b/drivers/net/wireless/mediatek/mt76/sdio.c
> > @@ -479,7 +479,8 @@ static void mt76s_status_worker(struct mt76_worker *w)
> >  			resched = true;
> >  
> >  		if (dev->drv->tx_status_data &&
> > -		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state))
> > +		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state) &&
> > +		    !test_bit(MT76_STATE_SUSPEND, &dev->phy.state))
> >  			queue_work(dev->wq, &dev->sdio.stat_work);
> >  	} while (nframes > 0);
> >  
> > diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> > index 649a56790b89..801590a0a334 100644
> > --- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> > +++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> > @@ -317,7 +317,8 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
> >  		if (ret > 0)
> >  			nframes += ret;
> >  
> > -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
> > +		if (test_bit(MT76_MCU_RESET, &dev->phy.state) ||
> > +		    test_bit(MT76_STATE_SUSPEND, &dev->phy.state)) {
> >  			if (!mt76s_txqs_empty(dev))
> >  				continue;
> >  			else
> 
> since mt76s_tx_run_queue will not run if MT76_MCU_RESET is set, do we really
> need to check it for each iteration or is fine something like:
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> index 649a56790b89..68f30a83fa5d 100644
> --- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> +++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> @@ -317,14 +317,12 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
>  		if (ret > 0)
>  			nframes += ret;
>  
> -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
> -			if (!mt76s_txqs_empty(dev))
> -				continue;
> -			else
> -				wake_up(&sdio->wait);
> -		}
>  	} while (nframes > 0);
>  
> +	if (test_bit(MT76_MCU_RESET, &dev->phy.state) &&
> +	    mt76s_txqs_empty(dev))
> +		wake_up(&sdio->wait);
> +
>  	/* enable interrupt */
>  	sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
>  	sdio_release_host(sdio->func);
> 
> Regards,
> Lorenzo
> 
> > -- 
> > 2.25.1
> > 



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend

[Lorenzo Bianconi]

[-- Attachment #1.1: Type: text/plain, Size: 8628 bytes --]

> > From: Sean Wang <sean.wang@mediatek.com>
> > 
> > According to the MT7921S firmware, the cmd MCU_UNI_CMD_HIF_CTRL have to
> > be last MCU command to execute in suspend handler and all data traffic
> > have to be stopped before the cmd MCU_UNI_CMD_HIF_CTRL starts as well
> > in order that mt7921 can successfully fall into the deep sleep mode.
> > 
> > Where we reuse the flag MT76_STATE_SUSPEND and avoid creating
> > another global flag to stop all of the traffic onto the SDIO bus.
> > 
> > Fixes: 48fab5bbef40 ("mt76: mt7921: introduce mt7921s support")
> > Reported-by: Leon Yen <leon.yen@mediatek.com>
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > ---
> >  .../wireless/mediatek/mt76/mt76_connac_mcu.c  |  2 +-
> >  .../net/wireless/mediatek/mt76/mt7921/main.c  |  3 --
> >  .../net/wireless/mediatek/mt76/mt7921/sdio.c  | 34 ++++++++++++-------
> >  drivers/net/wireless/mediatek/mt76/sdio.c     |  3 +-
> >  .../net/wireless/mediatek/mt76/sdio_txrx.c    |  3 +-
> >  5 files changed, 27 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> > index 26b4b875dcc0..61c4c86e79c8 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> > @@ -2461,7 +2461,7 @@ void mt76_connac_mcu_set_suspend_iter(void *priv, u8 *mac,
> >  				      struct ieee80211_vif *vif)
> >  {
> >  	struct mt76_phy *phy = priv;
> > -	bool suspend = test_bit(MT76_STATE_SUSPEND, &phy->state);
> > +	bool suspend = !test_bit(MT76_STATE_RUNNING, &phy->state);
> >  	struct ieee80211_hw *hw = phy->hw;
> >  	struct cfg80211_wowlan *wowlan = hw->wiphy->wowlan_config;
> >  	int i;
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/main.c b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> > index e022251b4006..0b2a6b7f22ea 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> > @@ -1242,8 +1242,6 @@ static int mt7921_suspend(struct ieee80211_hw *hw,
> >  	mt7921_mutex_acquire(dev);
> >  
> >  	clear_bit(MT76_STATE_RUNNING, &phy->mt76->state);
> > -
> > -	set_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
> >  	ieee80211_iterate_active_interfaces(hw,
> >  					    IEEE80211_IFACE_ITER_RESUME_ALL,
> >  					    mt76_connac_mcu_set_suspend_iter,
> > @@ -1262,7 +1260,6 @@ static int mt7921_resume(struct ieee80211_hw *hw)
> >  	mt7921_mutex_acquire(dev);
> >  
> >  	set_bit(MT76_STATE_RUNNING, &phy->mt76->state);
> > -	clear_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
> >  	ieee80211_iterate_active_interfaces(hw,
> >  					    IEEE80211_IFACE_ITER_RESUME_ALL,
> >  					    mt76_connac_mcu_set_suspend_iter,
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> > index 5fee489c7a99..5c88b6b8d097 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> > @@ -206,6 +206,8 @@ static int mt7921s_suspend(struct device *__dev)
> >  	int err;
> >  
> >  	pm->suspended = true;
> > +	set_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
> > +
> >  	cancel_delayed_work_sync(&pm->ps_work);
> >  	cancel_work_sync(&pm->wake_work);
> >  
> > @@ -213,10 +215,6 @@ static int mt7921s_suspend(struct device *__dev)
> >  	if (err < 0)
> >  		goto restore_suspend;
> >  
> > -	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
> > -	if (err)
> > -		goto restore_suspend;
> > -
> >  	/* always enable deep sleep during suspend to reduce
> >  	 * power consumption
> >  	 */
> > @@ -224,34 +222,45 @@ static int mt7921s_suspend(struct device *__dev)
> >  
> >  	mt76_txq_schedule_all(&dev->mphy);
> >  	mt76_worker_disable(&mdev->tx_worker);
> > -	mt76_worker_disable(&mdev->sdio.txrx_worker);
> >  	mt76_worker_disable(&mdev->sdio.status_worker);
> > -	mt76_worker_disable(&mdev->sdio.net_worker);
> >  	cancel_work_sync(&mdev->sdio.stat_work);
> >  	clear_bit(MT76_READING_STATS, &dev->mphy.state);
> > -
> >  	mt76_tx_status_check(mdev, true);
> >  
> > -	err = mt7921_mcu_fw_pmctrl(dev);
> > +	mt76_worker_schedule(&mdev->sdio.txrx_worker);
> 
> I guess mt76_worker_disable() is supposed to do it, right?
> I guess we can assume txrx_worker is no longer running here, right?

I can see it now, txrx_worker can be running on the different cpu.
I guess we need to add just the wait_event_timeout() and move
mt76_connac_mcu_set_hif_suspend() below. What do you think?
Can you please try the chunk below?

Regards,
Lorenzo

> 
> > +	wait_event_timeout(dev->mt76.sdio.wait,
> > +			   mt76s_txqs_empty(&dev->mt76), 5 * HZ);
> > +
> > +	/* It is supposed that SDIO bus is idle at the point */
> > +	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
> >  	if (err)
> >  		goto restore_worker;
> >  
> > +	mt76_worker_disable(&mdev->sdio.txrx_worker);
> > +	mt76_worker_disable(&mdev->sdio.net_worker);
> > +
> > +	err = mt7921_mcu_fw_pmctrl(dev);
> > +	if (err)
> > +		goto restore_txrx_worker;
> > +
> >  	sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
> >  
> >  	return 0;
> >  
> > +restore_txrx_worker:
> > +	mt76_worker_enable(&mdev->sdio.net_worker);
> > +	mt76_worker_enable(&mdev->sdio.txrx_worker);
> > +	mt76_connac_mcu_set_hif_suspend(mdev, false);
> > +
> >  restore_worker:
> >  	mt76_worker_enable(&mdev->tx_worker);
> > -	mt76_worker_enable(&mdev->sdio.txrx_worker);
> >  	mt76_worker_enable(&mdev->sdio.status_worker);
> > -	mt76_worker_enable(&mdev->sdio.net_worker);
> >  
> >  	if (!pm->ds_enable)
> >  		mt76_connac_mcu_set_deep_sleep(mdev, false);
> >  
> > -	mt76_connac_mcu_set_hif_suspend(mdev, false);
> > -
> >  restore_suspend:
> > +	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
> >  	pm->suspended = false;
> >  
> >  	return err;
> > @@ -266,6 +275,7 @@ static int mt7921s_resume(struct device *__dev)
> >  	int err;
> >  
> >  	pm->suspended = false;
> > +	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
> >  
> >  	err = mt7921_mcu_drv_pmctrl(dev);
> >  	if (err < 0)
> > diff --git a/drivers/net/wireless/mediatek/mt76/sdio.c b/drivers/net/wireless/mediatek/mt76/sdio.c
> > index c99acc21225e..b0bc7be0fb1f 100644
> > --- a/drivers/net/wireless/mediatek/mt76/sdio.c
> > +++ b/drivers/net/wireless/mediatek/mt76/sdio.c
> > @@ -479,7 +479,8 @@ static void mt76s_status_worker(struct mt76_worker *w)
> >  			resched = true;
> >  
> >  		if (dev->drv->tx_status_data &&
> > -		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state))
> > +		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state) &&
> > +		    !test_bit(MT76_STATE_SUSPEND, &dev->phy.state))
> >  			queue_work(dev->wq, &dev->sdio.stat_work);
> >  	} while (nframes > 0);
> >  
> > diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> > index 649a56790b89..801590a0a334 100644
> > --- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> > +++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> > @@ -317,7 +317,8 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
> >  		if (ret > 0)
> >  			nframes += ret;
> >  
> > -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
> > +		if (test_bit(MT76_MCU_RESET, &dev->phy.state) ||
> > +		    test_bit(MT76_STATE_SUSPEND, &dev->phy.state)) {
> >  			if (!mt76s_txqs_empty(dev))
> >  				continue;
> >  			else
> 
> since mt76s_tx_run_queue will not run if MT76_MCU_RESET is set, do we really
> need to check it for each iteration or is fine something like:
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> index 649a56790b89..68f30a83fa5d 100644
> --- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> +++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> @@ -317,14 +317,12 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
>  		if (ret > 0)
>  			nframes += ret;
>  
> -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
> -			if (!mt76s_txqs_empty(dev))
> -				continue;
> -			else
> -				wake_up(&sdio->wait);
> -		}
>  	} while (nframes > 0);
>  
> +	if (test_bit(MT76_MCU_RESET, &dev->phy.state) &&
> +	    mt76s_txqs_empty(dev))
> +		wake_up(&sdio->wait);
> +
>  	/* enable interrupt */
>  	sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
>  	sdio_release_host(sdio->func);
> 
> Regards,
> Lorenzo
> 
> > -- 
> > 2.25.1
> > 



[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 7956 bytes --]

> From: Sean Wang <sean.wang@mediatek.com>
> 
> According to the MT7921S firmware, the cmd MCU_UNI_CMD_HIF_CTRL have to
> be last MCU command to execute in suspend handler and all data traffic
> have to be stopped before the cmd MCU_UNI_CMD_HIF_CTRL starts as well
> in order that mt7921 can successfully fall into the deep sleep mode.
> 
> Where we reuse the flag MT76_STATE_SUSPEND and avoid creating
> another global flag to stop all of the traffic onto the SDIO bus.
> 
> Fixes: 48fab5bbef40 ("mt76: mt7921: introduce mt7921s support")
> Reported-by: Leon Yen <leon.yen@mediatek.com>
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  .../wireless/mediatek/mt76/mt76_connac_mcu.c  |  2 +-
>  .../net/wireless/mediatek/mt76/mt7921/main.c  |  3 --
>  .../net/wireless/mediatek/mt76/mt7921/sdio.c  | 34 ++++++++++++-------
>  drivers/net/wireless/mediatek/mt76/sdio.c     |  3 +-
>  .../net/wireless/mediatek/mt76/sdio_txrx.c    |  3 +-
>  5 files changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> index 26b4b875dcc0..61c4c86e79c8 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> @@ -2461,7 +2461,7 @@ void mt76_connac_mcu_set_suspend_iter(void *priv, u8 *mac,
>  				      struct ieee80211_vif *vif)
>  {
>  	struct mt76_phy *phy = priv;
> -	bool suspend = test_bit(MT76_STATE_SUSPEND, &phy->state);
> +	bool suspend = !test_bit(MT76_STATE_RUNNING, &phy->state);
>  	struct ieee80211_hw *hw = phy->hw;
>  	struct cfg80211_wowlan *wowlan = hw->wiphy->wowlan_config;
>  	int i;
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/main.c b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> index e022251b4006..0b2a6b7f22ea 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> @@ -1242,8 +1242,6 @@ static int mt7921_suspend(struct ieee80211_hw *hw,
>  	mt7921_mutex_acquire(dev);
>  
>  	clear_bit(MT76_STATE_RUNNING, &phy->mt76->state);
> -
> -	set_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
>  	ieee80211_iterate_active_interfaces(hw,
>  					    IEEE80211_IFACE_ITER_RESUME_ALL,
>  					    mt76_connac_mcu_set_suspend_iter,
> @@ -1262,7 +1260,6 @@ static int mt7921_resume(struct ieee80211_hw *hw)
>  	mt7921_mutex_acquire(dev);
>  
>  	set_bit(MT76_STATE_RUNNING, &phy->mt76->state);
> -	clear_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
>  	ieee80211_iterate_active_interfaces(hw,
>  					    IEEE80211_IFACE_ITER_RESUME_ALL,
>  					    mt76_connac_mcu_set_suspend_iter,
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> index 5fee489c7a99..5c88b6b8d097 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> @@ -206,6 +206,8 @@ static int mt7921s_suspend(struct device *__dev)
>  	int err;
>  
>  	pm->suspended = true;
> +	set_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
> +
>  	cancel_delayed_work_sync(&pm->ps_work);
>  	cancel_work_sync(&pm->wake_work);
>  
> @@ -213,10 +215,6 @@ static int mt7921s_suspend(struct device *__dev)
>  	if (err < 0)
>  		goto restore_suspend;
>  
> -	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
> -	if (err)
> -		goto restore_suspend;
> -
>  	/* always enable deep sleep during suspend to reduce
>  	 * power consumption
>  	 */
> @@ -224,34 +222,45 @@ static int mt7921s_suspend(struct device *__dev)
>  
>  	mt76_txq_schedule_all(&dev->mphy);
>  	mt76_worker_disable(&mdev->tx_worker);
> -	mt76_worker_disable(&mdev->sdio.txrx_worker);
>  	mt76_worker_disable(&mdev->sdio.status_worker);
> -	mt76_worker_disable(&mdev->sdio.net_worker);
>  	cancel_work_sync(&mdev->sdio.stat_work);
>  	clear_bit(MT76_READING_STATS, &dev->mphy.state);
> -
>  	mt76_tx_status_check(mdev, true);
>  
> -	err = mt7921_mcu_fw_pmctrl(dev);
> +	mt76_worker_schedule(&mdev->sdio.txrx_worker);

I guess mt76_worker_disable() is supposed to do it, right?
I guess we can assume txrx_worker is no longer running here, right?

> +	wait_event_timeout(dev->mt76.sdio.wait,
> +			   mt76s_txqs_empty(&dev->mt76), 5 * HZ);
> +
> +	/* It is supposed that SDIO bus is idle at the point */
> +	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
>  	if (err)
>  		goto restore_worker;
>  
> +	mt76_worker_disable(&mdev->sdio.txrx_worker);
> +	mt76_worker_disable(&mdev->sdio.net_worker);
> +
> +	err = mt7921_mcu_fw_pmctrl(dev);
> +	if (err)
> +		goto restore_txrx_worker;
> +
>  	sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
>  
>  	return 0;
>  
> +restore_txrx_worker:
> +	mt76_worker_enable(&mdev->sdio.net_worker);
> +	mt76_worker_enable(&mdev->sdio.txrx_worker);
> +	mt76_connac_mcu_set_hif_suspend(mdev, false);
> +
>  restore_worker:
>  	mt76_worker_enable(&mdev->tx_worker);
> -	mt76_worker_enable(&mdev->sdio.txrx_worker);
>  	mt76_worker_enable(&mdev->sdio.status_worker);
> -	mt76_worker_enable(&mdev->sdio.net_worker);
>  
>  	if (!pm->ds_enable)
>  		mt76_connac_mcu_set_deep_sleep(mdev, false);
>  
> -	mt76_connac_mcu_set_hif_suspend(mdev, false);
> -
>  restore_suspend:
> +	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
>  	pm->suspended = false;
>  
>  	return err;
> @@ -266,6 +275,7 @@ static int mt7921s_resume(struct device *__dev)
>  	int err;
>  
>  	pm->suspended = false;
> +	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
>  
>  	err = mt7921_mcu_drv_pmctrl(dev);
>  	if (err < 0)
> diff --git a/drivers/net/wireless/mediatek/mt76/sdio.c b/drivers/net/wireless/mediatek/mt76/sdio.c
> index c99acc21225e..b0bc7be0fb1f 100644
> --- a/drivers/net/wireless/mediatek/mt76/sdio.c
> +++ b/drivers/net/wireless/mediatek/mt76/sdio.c
> @@ -479,7 +479,8 @@ static void mt76s_status_worker(struct mt76_worker *w)
>  			resched = true;
>  
>  		if (dev->drv->tx_status_data &&
> -		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state))
> +		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state) &&
> +		    !test_bit(MT76_STATE_SUSPEND, &dev->phy.state))
>  			queue_work(dev->wq, &dev->sdio.stat_work);
>  	} while (nframes > 0);
>  
> diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> index 649a56790b89..801590a0a334 100644
> --- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> +++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> @@ -317,7 +317,8 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
>  		if (ret > 0)
>  			nframes += ret;
>  
> -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
> +		if (test_bit(MT76_MCU_RESET, &dev->phy.state) ||
> +		    test_bit(MT76_STATE_SUSPEND, &dev->phy.state)) {
>  			if (!mt76s_txqs_empty(dev))
>  				continue;
>  			else

since mt76s_tx_run_queue will not run if MT76_MCU_RESET is set, do we really
need to check it for each iteration or is fine something like:

diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
index 649a56790b89..68f30a83fa5d 100644
--- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
+++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
@@ -317,14 +317,12 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
 		if (ret > 0)
 			nframes += ret;
 
-		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
-			if (!mt76s_txqs_empty(dev))
-				continue;
-			else
-				wake_up(&sdio->wait);
-		}
 	} while (nframes > 0);
 
+	if (test_bit(MT76_MCU_RESET, &dev->phy.state) &&
+	    mt76s_txqs_empty(dev))
+		wake_up(&sdio->wait);
+
 	/* enable interrupt */
 	sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
 	sdio_release_host(sdio->func);

Regards,
Lorenzo

> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend

[Lorenzo Bianconi]

[-- Attachment #1.1: Type: text/plain, Size: 7956 bytes --]

> From: Sean Wang <sean.wang@mediatek.com>
> 
> According to the MT7921S firmware, the cmd MCU_UNI_CMD_HIF_CTRL have to
> be last MCU command to execute in suspend handler and all data traffic
> have to be stopped before the cmd MCU_UNI_CMD_HIF_CTRL starts as well
> in order that mt7921 can successfully fall into the deep sleep mode.
> 
> Where we reuse the flag MT76_STATE_SUSPEND and avoid creating
> another global flag to stop all of the traffic onto the SDIO bus.
> 
> Fixes: 48fab5bbef40 ("mt76: mt7921: introduce mt7921s support")
> Reported-by: Leon Yen <leon.yen@mediatek.com>
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  .../wireless/mediatek/mt76/mt76_connac_mcu.c  |  2 +-
>  .../net/wireless/mediatek/mt76/mt7921/main.c  |  3 --
>  .../net/wireless/mediatek/mt76/mt7921/sdio.c  | 34 ++++++++++++-------
>  drivers/net/wireless/mediatek/mt76/sdio.c     |  3 +-
>  .../net/wireless/mediatek/mt76/sdio_txrx.c    |  3 +-
>  5 files changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> index 26b4b875dcc0..61c4c86e79c8 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
> @@ -2461,7 +2461,7 @@ void mt76_connac_mcu_set_suspend_iter(void *priv, u8 *mac,
>  				      struct ieee80211_vif *vif)
>  {
>  	struct mt76_phy *phy = priv;
> -	bool suspend = test_bit(MT76_STATE_SUSPEND, &phy->state);
> +	bool suspend = !test_bit(MT76_STATE_RUNNING, &phy->state);
>  	struct ieee80211_hw *hw = phy->hw;
>  	struct cfg80211_wowlan *wowlan = hw->wiphy->wowlan_config;
>  	int i;
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/main.c b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> index e022251b4006..0b2a6b7f22ea 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> @@ -1242,8 +1242,6 @@ static int mt7921_suspend(struct ieee80211_hw *hw,
>  	mt7921_mutex_acquire(dev);
>  
>  	clear_bit(MT76_STATE_RUNNING, &phy->mt76->state);
> -
> -	set_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
>  	ieee80211_iterate_active_interfaces(hw,
>  					    IEEE80211_IFACE_ITER_RESUME_ALL,
>  					    mt76_connac_mcu_set_suspend_iter,
> @@ -1262,7 +1260,6 @@ static int mt7921_resume(struct ieee80211_hw *hw)
>  	mt7921_mutex_acquire(dev);
>  
>  	set_bit(MT76_STATE_RUNNING, &phy->mt76->state);
> -	clear_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
>  	ieee80211_iterate_active_interfaces(hw,
>  					    IEEE80211_IFACE_ITER_RESUME_ALL,
>  					    mt76_connac_mcu_set_suspend_iter,
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> index 5fee489c7a99..5c88b6b8d097 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> @@ -206,6 +206,8 @@ static int mt7921s_suspend(struct device *__dev)
>  	int err;
>  
>  	pm->suspended = true;
> +	set_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
> +
>  	cancel_delayed_work_sync(&pm->ps_work);
>  	cancel_work_sync(&pm->wake_work);
>  
> @@ -213,10 +215,6 @@ static int mt7921s_suspend(struct device *__dev)
>  	if (err < 0)
>  		goto restore_suspend;
>  
> -	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
> -	if (err)
> -		goto restore_suspend;
> -
>  	/* always enable deep sleep during suspend to reduce
>  	 * power consumption
>  	 */
> @@ -224,34 +222,45 @@ static int mt7921s_suspend(struct device *__dev)
>  
>  	mt76_txq_schedule_all(&dev->mphy);
>  	mt76_worker_disable(&mdev->tx_worker);
> -	mt76_worker_disable(&mdev->sdio.txrx_worker);
>  	mt76_worker_disable(&mdev->sdio.status_worker);
> -	mt76_worker_disable(&mdev->sdio.net_worker);
>  	cancel_work_sync(&mdev->sdio.stat_work);
>  	clear_bit(MT76_READING_STATS, &dev->mphy.state);
> -
>  	mt76_tx_status_check(mdev, true);
>  
> -	err = mt7921_mcu_fw_pmctrl(dev);
> +	mt76_worker_schedule(&mdev->sdio.txrx_worker);

I guess mt76_worker_disable() is supposed to do it, right?
I guess we can assume txrx_worker is no longer running here, right?

> +	wait_event_timeout(dev->mt76.sdio.wait,
> +			   mt76s_txqs_empty(&dev->mt76), 5 * HZ);
> +
> +	/* It is supposed that SDIO bus is idle at the point */
> +	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
>  	if (err)
>  		goto restore_worker;
>  
> +	mt76_worker_disable(&mdev->sdio.txrx_worker);
> +	mt76_worker_disable(&mdev->sdio.net_worker);
> +
> +	err = mt7921_mcu_fw_pmctrl(dev);
> +	if (err)
> +		goto restore_txrx_worker;
> +
>  	sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
>  
>  	return 0;
>  
> +restore_txrx_worker:
> +	mt76_worker_enable(&mdev->sdio.net_worker);
> +	mt76_worker_enable(&mdev->sdio.txrx_worker);
> +	mt76_connac_mcu_set_hif_suspend(mdev, false);
> +
>  restore_worker:
>  	mt76_worker_enable(&mdev->tx_worker);
> -	mt76_worker_enable(&mdev->sdio.txrx_worker);
>  	mt76_worker_enable(&mdev->sdio.status_worker);
> -	mt76_worker_enable(&mdev->sdio.net_worker);
>  
>  	if (!pm->ds_enable)
>  		mt76_connac_mcu_set_deep_sleep(mdev, false);
>  
> -	mt76_connac_mcu_set_hif_suspend(mdev, false);
> -
>  restore_suspend:
> +	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
>  	pm->suspended = false;
>  
>  	return err;
> @@ -266,6 +275,7 @@ static int mt7921s_resume(struct device *__dev)
>  	int err;
>  
>  	pm->suspended = false;
> +	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
>  
>  	err = mt7921_mcu_drv_pmctrl(dev);
>  	if (err < 0)
> diff --git a/drivers/net/wireless/mediatek/mt76/sdio.c b/drivers/net/wireless/mediatek/mt76/sdio.c
> index c99acc21225e..b0bc7be0fb1f 100644
> --- a/drivers/net/wireless/mediatek/mt76/sdio.c
> +++ b/drivers/net/wireless/mediatek/mt76/sdio.c
> @@ -479,7 +479,8 @@ static void mt76s_status_worker(struct mt76_worker *w)
>  			resched = true;
>  
>  		if (dev->drv->tx_status_data &&
> -		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state))
> +		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state) &&
> +		    !test_bit(MT76_STATE_SUSPEND, &dev->phy.state))
>  			queue_work(dev->wq, &dev->sdio.stat_work);
>  	} while (nframes > 0);
>  
> diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> index 649a56790b89..801590a0a334 100644
> --- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> +++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> @@ -317,7 +317,8 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
>  		if (ret > 0)
>  			nframes += ret;
>  
> -		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
> +		if (test_bit(MT76_MCU_RESET, &dev->phy.state) ||
> +		    test_bit(MT76_STATE_SUSPEND, &dev->phy.state)) {
>  			if (!mt76s_txqs_empty(dev))
>  				continue;
>  			else

since mt76s_tx_run_queue will not run if MT76_MCU_RESET is set, do we really
need to check it for each iteration or is fine something like:

diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
index 649a56790b89..68f30a83fa5d 100644
--- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
+++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
@@ -317,14 +317,12 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
 		if (ret > 0)
 			nframes += ret;
 
-		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
-			if (!mt76s_txqs_empty(dev))
-				continue;
-			else
-				wake_up(&sdio->wait);
-		}
 	} while (nframes > 0);
 
+	if (test_bit(MT76_MCU_RESET, &dev->phy.state) &&
+	    mt76s_txqs_empty(dev))
+		wake_up(&sdio->wait);
+
 	/* enable interrupt */
 	sdio_writel(sdio->func, WHLPCR_INT_EN_SET, MCR_WHLPCR, NULL);
 	sdio_release_host(sdio->func);

Regards,
Lorenzo

> -- 
> 2.25.1
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

According to the MT7921S firmware, the cmd MCU_UNI_CMD_HIF_CTRL have to
be last MCU command to execute in suspend handler and all data traffic
have to be stopped before the cmd MCU_UNI_CMD_HIF_CTRL starts as well
in order that mt7921 can successfully fall into the deep sleep mode.

Where we reuse the flag MT76_STATE_SUSPEND and avoid creating
another global flag to stop all of the traffic onto the SDIO bus.

Fixes: 48fab5bbef40 ("mt76: mt7921: introduce mt7921s support")
Reported-by: Leon Yen <leon.yen@mediatek.com>
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 .../wireless/mediatek/mt76/mt76_connac_mcu.c  |  2 +-
 .../net/wireless/mediatek/mt76/mt7921/main.c  |  3 --
 .../net/wireless/mediatek/mt76/mt7921/sdio.c  | 34 ++++++++++++-------
 drivers/net/wireless/mediatek/mt76/sdio.c     |  3 +-
 .../net/wireless/mediatek/mt76/sdio_txrx.c    |  3 +-
 5 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
index 26b4b875dcc0..61c4c86e79c8 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
@@ -2461,7 +2461,7 @@ void mt76_connac_mcu_set_suspend_iter(void *priv, u8 *mac,
 				      struct ieee80211_vif *vif)
 {
 	struct mt76_phy *phy = priv;
-	bool suspend = test_bit(MT76_STATE_SUSPEND, &phy->state);
+	bool suspend = !test_bit(MT76_STATE_RUNNING, &phy->state);
 	struct ieee80211_hw *hw = phy->hw;
 	struct cfg80211_wowlan *wowlan = hw->wiphy->wowlan_config;
 	int i;
diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/main.c b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
index e022251b4006..0b2a6b7f22ea 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
@@ -1242,8 +1242,6 @@ static int mt7921_suspend(struct ieee80211_hw *hw,
 	mt7921_mutex_acquire(dev);
 
 	clear_bit(MT76_STATE_RUNNING, &phy->mt76->state);
-
-	set_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
 	ieee80211_iterate_active_interfaces(hw,
 					    IEEE80211_IFACE_ITER_RESUME_ALL,
 					    mt76_connac_mcu_set_suspend_iter,
@@ -1262,7 +1260,6 @@ static int mt7921_resume(struct ieee80211_hw *hw)
 	mt7921_mutex_acquire(dev);
 
 	set_bit(MT76_STATE_RUNNING, &phy->mt76->state);
-	clear_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
 	ieee80211_iterate_active_interfaces(hw,
 					    IEEE80211_IFACE_ITER_RESUME_ALL,
 					    mt76_connac_mcu_set_suspend_iter,
diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
index 5fee489c7a99..5c88b6b8d097 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
@@ -206,6 +206,8 @@ static int mt7921s_suspend(struct device *__dev)
 	int err;
 
 	pm->suspended = true;
+	set_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
+
 	cancel_delayed_work_sync(&pm->ps_work);
 	cancel_work_sync(&pm->wake_work);
 
@@ -213,10 +215,6 @@ static int mt7921s_suspend(struct device *__dev)
 	if (err < 0)
 		goto restore_suspend;
 
-	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
-	if (err)
-		goto restore_suspend;
-
 	/* always enable deep sleep during suspend to reduce
 	 * power consumption
 	 */
@@ -224,34 +222,45 @@ static int mt7921s_suspend(struct device *__dev)
 
 	mt76_txq_schedule_all(&dev->mphy);
 	mt76_worker_disable(&mdev->tx_worker);
-	mt76_worker_disable(&mdev->sdio.txrx_worker);
 	mt76_worker_disable(&mdev->sdio.status_worker);
-	mt76_worker_disable(&mdev->sdio.net_worker);
 	cancel_work_sync(&mdev->sdio.stat_work);
 	clear_bit(MT76_READING_STATS, &dev->mphy.state);
-
 	mt76_tx_status_check(mdev, true);
 
-	err = mt7921_mcu_fw_pmctrl(dev);
+	mt76_worker_schedule(&mdev->sdio.txrx_worker);
+	wait_event_timeout(dev->mt76.sdio.wait,
+			   mt76s_txqs_empty(&dev->mt76), 5 * HZ);
+
+	/* It is supposed that SDIO bus is idle at the point */
+	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
 	if (err)
 		goto restore_worker;
 
+	mt76_worker_disable(&mdev->sdio.txrx_worker);
+	mt76_worker_disable(&mdev->sdio.net_worker);
+
+	err = mt7921_mcu_fw_pmctrl(dev);
+	if (err)
+		goto restore_txrx_worker;
+
 	sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
 
 	return 0;
 
+restore_txrx_worker:
+	mt76_worker_enable(&mdev->sdio.net_worker);
+	mt76_worker_enable(&mdev->sdio.txrx_worker);
+	mt76_connac_mcu_set_hif_suspend(mdev, false);
+
 restore_worker:
 	mt76_worker_enable(&mdev->tx_worker);
-	mt76_worker_enable(&mdev->sdio.txrx_worker);
 	mt76_worker_enable(&mdev->sdio.status_worker);
-	mt76_worker_enable(&mdev->sdio.net_worker);
 
 	if (!pm->ds_enable)
 		mt76_connac_mcu_set_deep_sleep(mdev, false);
 
-	mt76_connac_mcu_set_hif_suspend(mdev, false);
-
 restore_suspend:
+	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
 	pm->suspended = false;
 
 	return err;
@@ -266,6 +275,7 @@ static int mt7921s_resume(struct device *__dev)
 	int err;
 
 	pm->suspended = false;
+	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
 
 	err = mt7921_mcu_drv_pmctrl(dev);
 	if (err < 0)
diff --git a/drivers/net/wireless/mediatek/mt76/sdio.c b/drivers/net/wireless/mediatek/mt76/sdio.c
index c99acc21225e..b0bc7be0fb1f 100644
--- a/drivers/net/wireless/mediatek/mt76/sdio.c
+++ b/drivers/net/wireless/mediatek/mt76/sdio.c
@@ -479,7 +479,8 @@ static void mt76s_status_worker(struct mt76_worker *w)
 			resched = true;
 
 		if (dev->drv->tx_status_data &&
-		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state))
+		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state) &&
+		    !test_bit(MT76_STATE_SUSPEND, &dev->phy.state))
 			queue_work(dev->wq, &dev->sdio.stat_work);
 	} while (nframes > 0);
 
diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
index 649a56790b89..801590a0a334 100644
--- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
+++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
@@ -317,7 +317,8 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
 		if (ret > 0)
 			nframes += ret;
 
-		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
+		if (test_bit(MT76_MCU_RESET, &dev->phy.state) ||
+		    test_bit(MT76_STATE_SUSPEND, &dev->phy.state)) {
 			if (!mt76s_txqs_empty(dev))
 				continue;
 			else
-- 
2.25.1

[PATCH 2/2] mt76: mt7921s: fix the device cannot sleep deeply in suspend

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

According to the MT7921S firmware, the cmd MCU_UNI_CMD_HIF_CTRL have to
be last MCU command to execute in suspend handler and all data traffic
have to be stopped before the cmd MCU_UNI_CMD_HIF_CTRL starts as well
in order that mt7921 can successfully fall into the deep sleep mode.

Where we reuse the flag MT76_STATE_SUSPEND and avoid creating
another global flag to stop all of the traffic onto the SDIO bus.

Fixes: 48fab5bbef40 ("mt76: mt7921: introduce mt7921s support")
Reported-by: Leon Yen <leon.yen@mediatek.com>
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 .../wireless/mediatek/mt76/mt76_connac_mcu.c  |  2 +-
 .../net/wireless/mediatek/mt76/mt7921/main.c  |  3 --
 .../net/wireless/mediatek/mt76/mt7921/sdio.c  | 34 ++++++++++++-------
 drivers/net/wireless/mediatek/mt76/sdio.c     |  3 +-
 .../net/wireless/mediatek/mt76/sdio_txrx.c    |  3 +-
 5 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
index 26b4b875dcc0..61c4c86e79c8 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
@@ -2461,7 +2461,7 @@ void mt76_connac_mcu_set_suspend_iter(void *priv, u8 *mac,
 				      struct ieee80211_vif *vif)
 {
 	struct mt76_phy *phy = priv;
-	bool suspend = test_bit(MT76_STATE_SUSPEND, &phy->state);
+	bool suspend = !test_bit(MT76_STATE_RUNNING, &phy->state);
 	struct ieee80211_hw *hw = phy->hw;
 	struct cfg80211_wowlan *wowlan = hw->wiphy->wowlan_config;
 	int i;
diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/main.c b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
index e022251b4006..0b2a6b7f22ea 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
@@ -1242,8 +1242,6 @@ static int mt7921_suspend(struct ieee80211_hw *hw,
 	mt7921_mutex_acquire(dev);
 
 	clear_bit(MT76_STATE_RUNNING, &phy->mt76->state);
-
-	set_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
 	ieee80211_iterate_active_interfaces(hw,
 					    IEEE80211_IFACE_ITER_RESUME_ALL,
 					    mt76_connac_mcu_set_suspend_iter,
@@ -1262,7 +1260,6 @@ static int mt7921_resume(struct ieee80211_hw *hw)
 	mt7921_mutex_acquire(dev);
 
 	set_bit(MT76_STATE_RUNNING, &phy->mt76->state);
-	clear_bit(MT76_STATE_SUSPEND, &phy->mt76->state);
 	ieee80211_iterate_active_interfaces(hw,
 					    IEEE80211_IFACE_ITER_RESUME_ALL,
 					    mt76_connac_mcu_set_suspend_iter,
diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
index 5fee489c7a99..5c88b6b8d097 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
@@ -206,6 +206,8 @@ static int mt7921s_suspend(struct device *__dev)
 	int err;
 
 	pm->suspended = true;
+	set_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
+
 	cancel_delayed_work_sync(&pm->ps_work);
 	cancel_work_sync(&pm->wake_work);
 
@@ -213,10 +215,6 @@ static int mt7921s_suspend(struct device *__dev)
 	if (err < 0)
 		goto restore_suspend;
 
-	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
-	if (err)
-		goto restore_suspend;
-
 	/* always enable deep sleep during suspend to reduce
 	 * power consumption
 	 */
@@ -224,34 +222,45 @@ static int mt7921s_suspend(struct device *__dev)
 
 	mt76_txq_schedule_all(&dev->mphy);
 	mt76_worker_disable(&mdev->tx_worker);
-	mt76_worker_disable(&mdev->sdio.txrx_worker);
 	mt76_worker_disable(&mdev->sdio.status_worker);
-	mt76_worker_disable(&mdev->sdio.net_worker);
 	cancel_work_sync(&mdev->sdio.stat_work);
 	clear_bit(MT76_READING_STATS, &dev->mphy.state);
-
 	mt76_tx_status_check(mdev, true);
 
-	err = mt7921_mcu_fw_pmctrl(dev);
+	mt76_worker_schedule(&mdev->sdio.txrx_worker);
+	wait_event_timeout(dev->mt76.sdio.wait,
+			   mt76s_txqs_empty(&dev->mt76), 5 * HZ);
+
+	/* It is supposed that SDIO bus is idle at the point */
+	err = mt76_connac_mcu_set_hif_suspend(mdev, true);
 	if (err)
 		goto restore_worker;
 
+	mt76_worker_disable(&mdev->sdio.txrx_worker);
+	mt76_worker_disable(&mdev->sdio.net_worker);
+
+	err = mt7921_mcu_fw_pmctrl(dev);
+	if (err)
+		goto restore_txrx_worker;
+
 	sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
 
 	return 0;
 
+restore_txrx_worker:
+	mt76_worker_enable(&mdev->sdio.net_worker);
+	mt76_worker_enable(&mdev->sdio.txrx_worker);
+	mt76_connac_mcu_set_hif_suspend(mdev, false);
+
 restore_worker:
 	mt76_worker_enable(&mdev->tx_worker);
-	mt76_worker_enable(&mdev->sdio.txrx_worker);
 	mt76_worker_enable(&mdev->sdio.status_worker);
-	mt76_worker_enable(&mdev->sdio.net_worker);
 
 	if (!pm->ds_enable)
 		mt76_connac_mcu_set_deep_sleep(mdev, false);
 
-	mt76_connac_mcu_set_hif_suspend(mdev, false);
-
 restore_suspend:
+	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
 	pm->suspended = false;
 
 	return err;
@@ -266,6 +275,7 @@ static int mt7921s_resume(struct device *__dev)
 	int err;
 
 	pm->suspended = false;
+	clear_bit(MT76_STATE_SUSPEND, &mdev->phy.state);
 
 	err = mt7921_mcu_drv_pmctrl(dev);
 	if (err < 0)
diff --git a/drivers/net/wireless/mediatek/mt76/sdio.c b/drivers/net/wireless/mediatek/mt76/sdio.c
index c99acc21225e..b0bc7be0fb1f 100644
--- a/drivers/net/wireless/mediatek/mt76/sdio.c
+++ b/drivers/net/wireless/mediatek/mt76/sdio.c
@@ -479,7 +479,8 @@ static void mt76s_status_worker(struct mt76_worker *w)
 			resched = true;
 
 		if (dev->drv->tx_status_data &&
-		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state))
+		    !test_and_set_bit(MT76_READING_STATS, &dev->phy.state) &&
+		    !test_bit(MT76_STATE_SUSPEND, &dev->phy.state))
 			queue_work(dev->wq, &dev->sdio.stat_work);
 	} while (nframes > 0);
 
diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
index 649a56790b89..801590a0a334 100644
--- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
+++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
@@ -317,7 +317,8 @@ void mt76s_txrx_worker(struct mt76_sdio *sdio)
 		if (ret > 0)
 			nframes += ret;
 
-		if (test_bit(MT76_MCU_RESET, &dev->phy.state)) {
+		if (test_bit(MT76_MCU_RESET, &dev->phy.state) ||
+		    test_bit(MT76_STATE_SUSPEND, &dev->phy.state)) {
 			if (!mt76s_txqs_empty(dev))
 				continue;
 			else
-- 
2.25.1


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek