Received-SPF: pass (google.com: domain of linux-kernel+bounces-88914-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 1 Mar 2024 19:20:22 +0100
From: Andi Shyti <andi.shyti@kernel.org>
To: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
Cc: konrad.dybcio@linaro.org, bjorn.andersson@linaro.org, vkoul@kernel.org, 
	wsa@kernel.org, linux-arm-msm@vger.kernel.org, dmaengine@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org, quic_vdadhani@quicinc.com
Subject: Re: [SPAM] [PATCH v1] i2c: i2c-qcom-geni: Parse Error correctly in
 i2c GSI mode
Message-ID: <zpkc4hxxkalw2cqhp5fbhdge4vhfrthi2ezhkpecninqvdcn6n@oddo2j6tabce>
References: <20240301112638.990045-1-quic_msavaliy@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240301112638.990045-1-quic_msavaliy@quicinc.com>

Hi Mukesh,

On Fri, Mar 01, 2024 at 04:56:38PM +0530, Mukesh Kumar Savaliya wrote:
> we are seeing protocol errors like NACK as transfer failure but

/we/We/

> ideally it should report exact error like NACK, BUS_PROTO or ARB_LOST.
> 
> Hence we are adding such error support in GSI mode and reporting it
> accordingly by adding respective error logs.
> 
> geni_i2c_gpi_xfer() needed to allocate heap based memory instead of

Please use the imperative form.

> stack memory to handle and store the geni_i2c_dev handle.
> 
> Copy event status from GSI driver to the i2c device status and parse
> error when callback comes from gsi driver to the i2c driver. In the
> gpi.c, we need to store callback param into i2c config data structure
> so that inside the i2c driver, we can check what exactly the error is
> and parse it accordingly.
> 
> Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")

What bug are you fixing here? The description doesn't talk about
fixes rather than support added.

..

> -	config.peripheral_config = &peripheral;
> -	config.peripheral_size = sizeof(peripheral);
> +	peripheral = devm_kzalloc(gi2c->se.dev, sizeof(*peripheral), GFP_KERNEL);

This is a massive leak. Why are you deciding to make the
allocation dynamic?

Thanks,
Andi