Date:   Sat, 12 Sep 2020 21:27:43 -0500
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Alex Elder <elder@...aro.org>
Cc:     davem@...emloft.net, kuba@...nel.org, evgreen@...omium.org,
        subashab@...eaurora.org, cpratapa@...eaurora.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 5/7] net: ipa: use device_init_wakeup()

On Fri 11 Sep 19:45 CDT 2020, Alex Elder wrote:

> The call to wakeup_source_register() in ipa_probe() does not do what
> it was intended to do.  Call device_init_wakeup() in ipa_setup()
> instead, to set the IPA device as wakeup-capable and to initially
> enable wakeup capability.
> 
> When we receive a SUSPEND interrupt, call pm_wakeup_dev_event()
> with a zero processing time, to simply call for a resume without
> any other processing.  The ipa_resume() call will take care of
> waking things up again, and will handle receiving the packet.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@...aro.org>

Regards,
Bjorn

> Signed-off-by: Alex Elder <elder@...aro.org>
> ---
>  drivers/net/ipa/ipa.h      |  2 --
>  drivers/net/ipa/ipa_main.c | 43 ++++++++++++++++----------------------
>  2 files changed, 18 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/ipa/ipa.h b/drivers/net/ipa/ipa.h
> index e02fe979b645b..c688155ccf375 100644
> --- a/drivers/net/ipa/ipa.h
> +++ b/drivers/net/ipa/ipa.h
> @@ -114,8 +114,6 @@ struct ipa {
>  	void *zero_virt;
>  	size_t zero_size;
>  
> -	struct wakeup_source *wakeup_source;
> -
>  	/* Bit masks indicating endpoint state */
>  	u32 available;		/* supported by hardware */
>  	u32 filter_map;
> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
> index 3b68b53c99015..5e714d9d2e5cb 100644
> --- a/drivers/net/ipa/ipa_main.c
> +++ b/drivers/net/ipa/ipa_main.c
> @@ -75,18 +75,19 @@
>   * @ipa:	IPA pointer
>   * @irq_id:	IPA interrupt type (unused)
>   *
> - * When in suspended state, the IPA can trigger a resume by sending a SUSPEND
> - * IPA interrupt.
> + * If an RX endpoint is in suspend state, and the IPA has a packet
> + * destined for that endpoint, the IPA generates a SUSPEND interrupt
> + * to inform the AP that it should resume the endpoint.  If we get
> + * one of these interrupts we just resume everything.
>   */
>  static void ipa_suspend_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
>  {
> -	/* Take a a single clock reference to prevent suspend.  All
> -	 * endpoints will be resumed as a result.  This reference will
> -	 * be dropped when we get a power management suspend request.
> -	 * The first call activates the clock; ignore any others.
> +	/* Just report the event, and let system resume handle the rest.
> +	 * More than one endpoint could signal this; if so, ignore
> +	 * all but the first.
>  	 */
>  	if (!test_and_set_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
> -		ipa_clock_get(ipa);
> +		pm_wakeup_dev_event(&ipa->pdev->dev, 0, true);
>  
>  	/* Acknowledge/clear the suspend interrupt on all endpoints */
>  	ipa_interrupt_suspend_clear_all(ipa->interrupt);
> @@ -107,6 +108,7 @@ int ipa_setup(struct ipa *ipa)
>  {
>  	struct ipa_endpoint *exception_endpoint;
>  	struct ipa_endpoint *command_endpoint;
> +	struct device *dev = &ipa->pdev->dev;
>  	int ret;
>  
>  	/* Setup for IPA v3.5.1 has some slight differences */
> @@ -124,6 +126,10 @@ int ipa_setup(struct ipa *ipa)
>  
>  	ipa_uc_setup(ipa);
>  
> +	ret = device_init_wakeup(dev, true);
> +	if (ret)
> +		goto err_uc_teardown;
> +
>  	ipa_endpoint_setup(ipa);
>  
>  	/* We need to use the AP command TX endpoint to perform other
> @@ -159,7 +165,7 @@ int ipa_setup(struct ipa *ipa)
>  
>  	ipa->setup_complete = true;
>  
> -	dev_info(&ipa->pdev->dev, "IPA driver setup completed successfully\n");
> +	dev_info(dev, "IPA driver setup completed successfully\n");
>  
>  	return 0;
>  
> @@ -174,6 +180,8 @@ int ipa_setup(struct ipa *ipa)
>  	ipa_endpoint_disable_one(command_endpoint);
>  err_endpoint_teardown:
>  	ipa_endpoint_teardown(ipa);
> +	(void)device_init_wakeup(dev, false);
> +err_uc_teardown:
>  	ipa_uc_teardown(ipa);
>  	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
>  	ipa_interrupt_teardown(ipa->interrupt);
> @@ -201,6 +209,7 @@ static void ipa_teardown(struct ipa *ipa)
>  	command_endpoint = ipa->name_map[IPA_ENDPOINT_AP_COMMAND_TX];
>  	ipa_endpoint_disable_one(command_endpoint);
>  	ipa_endpoint_teardown(ipa);
> +	(void)device_init_wakeup(&ipa->pdev->dev, false);
>  	ipa_uc_teardown(ipa);
>  	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
>  	ipa_interrupt_teardown(ipa->interrupt);
> @@ -715,7 +724,6 @@ static void ipa_validate_build(void)
>   */
>  static int ipa_probe(struct platform_device *pdev)
>  {
> -	struct wakeup_source *wakeup_source;
>  	struct device *dev = &pdev->dev;
>  	const struct ipa_data *data;
>  	struct ipa_clock *clock;
> @@ -764,19 +772,11 @@ static int ipa_probe(struct platform_device *pdev)
>  		goto err_clock_exit;
>  	}
>  
> -	/* Create a wakeup source. */
> -	wakeup_source = wakeup_source_register(dev, "ipa");
> -	if (!wakeup_source) {
> -		/* The most likely reason for failure is memory exhaustion */
> -		ret = -ENOMEM;
> -		goto err_clock_exit;
> -	}
> -
>  	/* Allocate and initialize the IPA structure */
>  	ipa = kzalloc(sizeof(*ipa), GFP_KERNEL);
>  	if (!ipa) {
>  		ret = -ENOMEM;
> -		goto err_wakeup_source_unregister;
> +		goto err_clock_exit;
>  	}
>  
>  	ipa->pdev = pdev;
> @@ -784,7 +784,6 @@ static int ipa_probe(struct platform_device *pdev)
>  	ipa->modem_rproc = rproc;
>  	ipa->clock = clock;
>  	__clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
> -	ipa->wakeup_source = wakeup_source;
>  	ipa->version = data->version;
>  
>  	ret = ipa_reg_init(ipa);
> @@ -863,8 +862,6 @@ static int ipa_probe(struct platform_device *pdev)
>  	ipa_reg_exit(ipa);
>  err_kfree_ipa:
>  	kfree(ipa);
> -err_wakeup_source_unregister:
> -	wakeup_source_unregister(wakeup_source);
>  err_clock_exit:
>  	ipa_clock_exit(clock);
>  err_rproc_put:
> @@ -878,11 +875,8 @@ static int ipa_remove(struct platform_device *pdev)
>  	struct ipa *ipa = dev_get_drvdata(&pdev->dev);
>  	struct rproc *rproc = ipa->modem_rproc;
>  	struct ipa_clock *clock = ipa->clock;
> -	struct wakeup_source *wakeup_source;
>  	int ret;
>  
> -	wakeup_source = ipa->wakeup_source;
> -
>  	if (ipa->setup_complete) {
>  		ret = ipa_modem_stop(ipa);
>  		if (ret)
> @@ -899,7 +893,6 @@ static int ipa_remove(struct platform_device *pdev)
>  	ipa_mem_exit(ipa);
>  	ipa_reg_exit(ipa);
>  	kfree(ipa);
> -	wakeup_source_unregister(wakeup_source);
>  	ipa_clock_exit(clock);
>  	rproc_put(rproc);
>  
> -- 
> 2.20.1
> 

Powered by blists - more mailing lists