Re: [PATCH] scsi: qla2xxx: Remove problematic BUILD_BUG_ON() assertion

[Finn Thain]

On Fri, 6 Mar 2026, Geert Uytterhoeven wrote:

> > I don't know of a good way to encode an invariant like "the last 
> > member of struct qla_tgt_sess_op is named atio" such that it might be 
> > statically checked. But perhaps there is a good way to do that (?)
> 
> Keeping the BUILD_BUG_ON(), but adding "__aligned(8);" to the ratio 
> member, as suggested by Arnd, would do that?
> 

No, that would merely limit the possibilities for tail padding in the 
future. Again, the tail padding is harmless. The assertion is bogus, not 
the struct layout.

Like I said to Arnd in that thread, the kind of twisted logic that would 
add an alignment rule here requires a comment to explain it (there is an 
example of this elsewhere in the same driver).

So now we're writing comments to explain code that exists solely to 
support a spurious assertion, which itself exists solely to allow an 
inadequate checker to prevent accidents that were already prevented by the 
warning in the comments. Why? Fear of regression.

It's true what they say -- "fear is the mind killer".

> > There's no Fixes tag here because there's no need to backport.
> > The BUILD_BUG_ON() comes from commit 091719c21d5a ("scsi: qla2xxx: target:
> > Fix invalid memory access with big CDBs") which appeared in v6.19-rc1.
> > The build failure first appeared in v7.0-rc1 with commit e428b013d9df
> > ("atomic: specify alignment for atomic_t and atomic64_t").
> 
> I would add both in Fixes, just in case anyone ever wants to backport
> e428b013d9df (which looks like a valid bugfix to me).
> 

Good point. I will add both.

> > --- a/drivers/scsi/qla2xxx/qla_target.c
> > +++ b/drivers/scsi/qla2xxx/qla_target.c
> > @@ -213,7 +213,6 @@ static void qlt_queue_unknown_atio(scsi_qla_host_t *vha,
> >         unsigned int add_cdb_len = 0;
> >
> >         /* atio must be the last member of qla_tgt_sess_op for add_cdb_len */
> > -       BUILD_BUG_ON(offsetof(struct qla_tgt_sess_op, atio) + sizeof(u->atio) != sizeof(*u));
> 
> Iff you remove the BUILD_BUG_ON(), you should remove the comment, too.
> 

Again, the comment refers to an assertion about the name of the last 
member. The BUILD_BUG_ON is an assertion about the size of the struct. 
These are not the same thing. If they were, I could agree with you: 
"remove both or neither".

Thanks for your review.