Re: [PATCH v2 2/2] iio: pressure: bmp280: Use char instead of s32 for data buffer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> > > -	s32 sensor_data[6] __aligned(8);
> > > +	struct {
> > > +		u8 buf[12];
> > > +		aligned_s64 ts;  
> > 
> > I'd missed that this depends on the number of channels.  It makes no functional
> > difference because the core code will happily write over the end of buf, but
> > from a representation point of view this might be
> > 
> > 		u8 buf[8];
> > 		aligned_s64 ts;
> > or
> > 		u8 buf[12];
> > 		aligned_s64 ts;
> > 
> > So given we can't actually fix on one or the other normal convention is
> > to just use something that forces a large enough aligned u8 buffer like
> > 		u8 buf[ALIGN(sizeof(s32) * BMP280_MAX_CHANNELS, 8) + sizeof(s64)]
> > 			__aligned(sizeof(s64));
> > 
> > Sorry for leading you astray on this!
> > 
> > Jonathan
> > 
> >   
> 
> I see your point. I can fix it in next version!
> 
> This is a neat issue here that requires indeed extra attention since
> this type of buffers is used basically by the majority of the drivers.
> Some type of runtime check in those registers would have been very
> very helpful, but I can't see of an easy way of doing it in the core
> code..
Adding size description has been on my todo list for a while so as to allow
the kernel to check the buffer is big enough and get rid of subtle overrun
bugs due to that oddity of the buffer needing to include the timestamp
space even though it's not obvious it will be written to.
That would also allow us to check alignment.  What we can't do is finer
grained checking of these structures but arguably we don't want to as this
is an elegance not correctness issue!

> 
> Thanks for the review :)
> 
> Cheers,
> Vasilis
> 
> > > +	} buffer;
> > >  
> > >  	/*
> > >  	 * DMA (thus cache coherency maintenance) may require the  
> >   





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux