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;
From:   tien.sung.ang@intel.com
To:     yilun.xu@intel.com
Cc:     christophe.jaillet@wanadoo.fr, hao.wu@intel.com,
        linux-fpga@vger.kernel.org, linux-kernel@vger.kernel.org,
        mdf@kernel.org, tien.sung.ang@intel.com, trix@redhat.com
Subject: Re: [PATCH v2] fpga: altera-cvp: Truncated bitstream error support
Date:   Tue,  7 Jun 2022 13:55:30 +0800
Message-Id: <20220607055530.3755617-1-tien.sung.ang@intel.com>
In-Reply-To: <20220603094911.GA238410@yilunxu-OptiPlex-7050>
References: <20220603094911.GA238410@yilunxu-OptiPlex-7050>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

> 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
insisted that the buffer be padded with whatever value. They
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?