On Tue, Apr 16, 2024 at 11:27:24AM +0530, Nikunj A. Dadhania wrote:
> > Why does that snp_get_os_area_msg_seqno() returns a pointer when you
> > deref it here again?
> >
> > A function which returns a sequence number should return that number
> > - not a pointer to it.
> >
> > Which then makes that u32 *os_area_msg_seqno redundant and you can use
> > the function directly.
> >
> > IOW:
> >
> > static inline u32 snp_get_os_area_msg_seqno(struct snp_guest_dev *snp_dev)
> > {
> > return snp_dev->layout->os_area.msg_seqno_0 + snp_dev->vmpck_id;
>
> This patch removes setting of layour page in snp_dev structure.
So?
> static inline u32 snp_get_os_area_msg_seqno(struct snp_guest_dev *snp_dev)
> {
> if (!platform_data)
> return NULL;
>
> return *(&platform_data->layout->os_area.msg_seqno_0 + vmpck_id);
> }
What?!
This snp_get_os_area_msg_seqno() is a new function added by this patch.
> I had a getter for getting the os_area_msg_seqno pointer, probably not
> a good function name.
Probably you need to go back to the drawing board and think about how
this thing should look like.
> > Do you see the imbalance in the APIs?
>
> The msg_seqno should only be incremented by 2 (always), that was the reason to avoid a setter.
And what's wrong with the setter doing the incrementation so that
callers can't even get it wrong?
It sounds to me like you should redesign this sequence number handling
in a *separate* patch.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
