Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 8 Jun 2022 18:30:58 +0800
From:   Xu Yilun <yilun.xu@intel.com>
To:     tien.sung.ang@intel.com
Cc:     christophe.jaillet@wanadoo.fr, hao.wu@intel.com,
        linux-fpga@vger.kernel.org, linux-kernel@vger.kernel.org,
        mdf@kernel.org, trix@redhat.com
Subject: Re: [PATCH v2] fpga: altera-cvp: Truncated bitstream error support
Message-ID: <20220608103058.GG481269@yilunxu-OptiPlex-7050>
References: <20220603094911.GA238410@yilunxu-OptiPlex-7050>
 <20220607055530.3755617-1-tien.sung.ang@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220607055530.3755617-1-tien.sung.ang@intel.com>
Precedence: bulk

On Tue, Jun 07, 2022 at 01:55:30PM +0800, tien.sung.ang@intel.com wrote:
> > But you are adding the logic in altera_cvp_write() to keep the
> > correctness. What's the difference adding it somewhere else? And as I
> > can see, preventing the writing at the very beginning is a much cleaner
> > way.
> 
> Though without doubt, your solution is doable, I have a 
> discussion with the Intel architect and they insisted that
> the device driver must not make such a decision. The decision to 
> drop or accept a transfer is up to the firmware. The firmware

Why dropping or accepting is forbidden, but padding is allowed? If the
decision is no operation to the data, then padding should also be
forbidden.

> insisted that the buffer be padded with whatever value. They

If I understand right, the 2 decisions are conflicted. On one hand,
the driver should not care about the data. On another hand, the
driver should still care about the data for some rule.

So please firstly reach your agreement before making patches.

> desire the transfer protocol to be obeyed to ensure, that 
> there is no hard dependencies on the device driver if they
> do one day change the 4kbytes to some other smaller value.
> 
> > > > If the image size is larger than 1 Page but is not aligned to 1 Page,
> > > > will the reprogramming still fail?
> > > Yes, the reconfiguration will fail. The above tear-down  is to prevent
> > > that CvP Hardware/firmware in the FPGA from entering into a dead-lock.
> 
> > So if the image size is not aligned to 1 Page, the reprogram should fail
> > anyway, is it? Then I really recommend you fail the reprogramming at the
> > very beginning.
> 
> Same reasoning as above. We don't want the driver to make this
> decision.
> 
> > > >> +		altera_cvp_send_block(conf, (const u32 *)conf->send_buf,
> > > >> +				      conf->priv->block_size);
> > > 
> > > >If the len equals block_size, is the copy still needed?
> > > Actually, not required. But to maintain a simple design, we use
> > > a common flow for all so you can skip the check.
> 
> > I don't think so. We should make the code reasonable. Blindly copy the
> > whole buffer impacts all normal cases, and makes people confused. The
> > code seems shorter now, but not simpler, it takes people more time to
> > figure out why.
> 
> Yes, it could look confusing to other programmers. And, yes, the 
> padding doesn't matter. Let me relook into this. As the driver is 
> already re-tested by the silicon validatioin. 
> I want to avoid making any change as it
> would meant another couple of weeks of re-testing. 
> Can this be accepted as it is?

Sorry, I can't. Some clarification is needed.

Thanks,
Yilun