Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 7 Nov 2022 09:15:49 +0100
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Alexander Usyskin <alexander.usyskin@intel.com>
Cc:     Jani Nikula <jani.nikula@linux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        Tomas Winkler <tomas.winkler@intel.com>,
        Vitaly Lubart <vitaly.lubart@intel.com>,
        intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mei: add timeout to send
Message-ID: <Y2i+tY2tZzQnUFKC@kroah.com>
References: <20221103155534.1966589-1-alexander.usyskin@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221103155534.1966589-1-alexander.usyskin@intel.com>
Precedence: bulk

On Thu, Nov 03, 2022 at 05:55:34PM +0200, Alexander Usyskin wrote:
> When driver wakes up the firmware from the low power stand,
> it is sending a memory ready message.
> The send is done via synchronous/blocking function to ensure
> that firmware is in ready state. However firmware might be
> in unstable state and send might be block forever.
> To address this issue a timeout is added to blocking write command on
> the internal bus.
> 
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> ---
>  drivers/misc/mei/bus-fixup.c | 19 +++++++++++--------
>  drivers/misc/mei/bus.c       |  9 +++++----
>  drivers/misc/mei/client.c    | 21 +++++++++++++++++----
>  drivers/misc/mei/client.h    |  2 +-
>  drivers/misc/mei/main.c      |  2 +-
>  drivers/misc/mei/mei_dev.h   |  2 +-
>  6 files changed, 36 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/misc/mei/bus-fixup.c b/drivers/misc/mei/bus-fixup.c
> index 71fbf0bc8453..3174cad8a5cc 100644
> --- a/drivers/misc/mei/bus-fixup.c
> +++ b/drivers/misc/mei/bus-fixup.c
> @@ -128,7 +128,7 @@ static int mei_osver(struct mei_cl_device *cldev)
>  	os_ver = (struct mei_os_ver *)fwcaps->data;
>  	os_ver->os_type = OSTYPE_LINUX;
>  
> -	return __mei_cl_send(cldev->cl, buf, size, 0, mode);
> +	return __mei_cl_send(cldev->cl, buf, size, 0, mode, 0);

Ick, adding another parameter to a function is not helpful here, what
does 0 mean unless you look it up?

> --- a/drivers/misc/mei/bus.c
> +++ b/drivers/misc/mei/bus.c
> @@ -29,11 +29,12 @@
>   * @length: buffer length
>   * @vtag: virtual tag
>   * @mode: sending mode
> + * @timeout: send timeout for blocking writes, 0 for infinite timeout

What units are these in?  Jiffies?  seconds?  hours?  Don't make us
guess :(

Why not write a function that is mei_cl_send_timeout() that has the new
option?  That way it's obvious that the other ones are blocking, right?

thanks,

greg k-h