Re: linux-next: manual merge of the staging tree with the staging.current tree

Re: linux-next: manual merge of the staging tree with the staging.current tree

[Jonathan Cameron]

On Mon, 8 Apr 2019 13:01:21 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Apr 08, 2019 at 09:14:58AM +0100, Jonathan Cameron wrote:
> > On Mon, 8 Apr 2019 13:02:12 +1000
> > Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >   
> > > Hi all,
> > > 
> > > Today's linux-next merge of the staging tree got a conflict in:
> > > 
> > >   drivers/iio/industrialio-buffer.c
> > > 
> > > between commit:
> > > 
> > >   20ea39ef9f2f ("iio: Fix scan mask selection")
> > > 
> > > from the staging.current tree and commit:
> > > 
> > >   3862828a903d ("iio: buffer: Switch to bitmap_zalloc()")
> > > 
> > > from the staging tree.
> > > 
> > > I fixed it up (I just used the staging tree version) and can carry the
> > > fix as necessary. This is now fixed as far as linux-next is concerned,
> > > but any non trivial conflicts should be mentioned to your upstream
> > > maintainer when your tree is submitted for merging.  You may also want
> > > to consider cooperating with the maintainer of the conflicting tree to
> > > minimise any particularly complex conflicts.
> > >   
> > Thanks Stephen,
> > 
> > That is the correct resolution.  
> 
> I think it still misses the following fix:
> 
Hi Andy,

Is that actually a problem given it's copied over from buffer->scan_mask just after allocation?
The two masks are the same length so I don't think we have a problem with this one.
Am I missing something?

Jonathan


> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 3c7e7380d1c3..9c2d0c97ed24 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -320,7 +320,7 @@ static int iio_scan_mask_set(struct iio_dev *indio_dev,
>  	const unsigned long *mask;
>  	unsigned long *trialmask;
>  
> -	trialmask = bitmap_alloc(indio_dev->masklength, GFP_KERNEL);
> +	trialmask = bitmap_zalloc(indio_dev->masklength, GFP_KERNEL);
>  	if (trialmask == NULL)
>  		return -ENOMEM;
>  	if (!indio_dev->masklength) {
> 
> 

Re: linux-next: manual merge of the staging tree with the staging.current tree

[Andy Shevchenko]

On Mon, Apr 08, 2019 at 11:14:39AM +0100, Jonathan Cameron wrote:
> On Mon, 8 Apr 2019 13:01:21 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Apr 08, 2019 at 09:14:58AM +0100, Jonathan Cameron wrote:
> > > On Mon, 8 Apr 2019 13:02:12 +1000
> > > Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> > > > Today's linux-next merge of the staging tree got a conflict in:
> > > > 
> > > >   drivers/iio/industrialio-buffer.c
> > > > 
> > > > between commit:
> > > > 
> > > >   20ea39ef9f2f ("iio: Fix scan mask selection")
> > > > 
> > > > from the staging.current tree and commit:
> > > > 
> > > >   3862828a903d ("iio: buffer: Switch to bitmap_zalloc()")
> > > > 
> > > > from the staging tree.
> > > > 
> > > > I fixed it up (I just used the staging tree version) and can carry the
> > > > fix as necessary. This is now fixed as far as linux-next is concerned,
> > > > but any non trivial conflicts should be mentioned to your upstream
> > > > maintainer when your tree is submitted for merging.  You may also want
> > > > to consider cooperating with the maintainer of the conflicting tree to
> > > > minimise any particularly complex conflicts.
> > > >   
> > > Thanks Stephen,
> > > 
> > > That is the correct resolution.  
> > 
> > I think it still misses the following fix:

> Is that actually a problem given it's copied over from buffer->scan_mask just after allocation?
> The two masks are the same length so I don't think we have a problem with this one.
> Am I missing something?

Hmm... I didn't get why the commit 20ea39ef9f2f fixes anything.

-- 
With Best Regards,
Andy Shevchenko

Re: linux-next: manual merge of the staging tree with the staging.current tree

[Jonathan Cameron]

On Mon, 8 Apr 2019 13:34:37 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Apr 08, 2019 at 11:14:39AM +0100, Jonathan Cameron wrote:
> > On Mon, 8 Apr 2019 13:01:21 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:  
> > > On Mon, Apr 08, 2019 at 09:14:58AM +0100, Jonathan Cameron wrote:  
> > > > On Mon, 8 Apr 2019 13:02:12 +1000
> > > > Stephen Rothwell <sfr@canb.auug.org.au> wrote:  
> 
> > > > > Today's linux-next merge of the staging tree got a conflict in:
> > > > > 
> > > > >   drivers/iio/industrialio-buffer.c
> > > > > 
> > > > > between commit:
> > > > > 
> > > > >   20ea39ef9f2f ("iio: Fix scan mask selection")
> > > > > 
> > > > > from the staging.current tree and commit:
> > > > > 
> > > > >   3862828a903d ("iio: buffer: Switch to bitmap_zalloc()")
> > > > > 
> > > > > from the staging tree.
> > > > > 
> > > > > I fixed it up (I just used the staging tree version) and can carry the
> > > > > fix as necessary. This is now fixed as far as linux-next is concerned,
> > > > > but any non trivial conflicts should be mentioned to your upstream
> > > > > maintainer when your tree is submitted for merging.  You may also want
> > > > > to consider cooperating with the maintainer of the conflicting tree to
> > > > > minimise any particularly complex conflicts.
> > > > >     
> > > > Thanks Stephen,
> > > > 
> > > > That is the correct resolution.    
> > > 
> > > I think it still misses the following fix:  
> 
> > Is that actually a problem given it's copied over from buffer->scan_mask just after allocation?
> > The two masks are the same length so I don't think we have a problem with this one.
> > Am I missing something?  
> 
> Hmm... I didn't get why the commit 20ea39ef9f2f fixes anything.
> 
Good point.  I'm don't think it ever did.  

Alex, any thoughts?

Jonathan

Re: linux-next: manual merge of the staging tree with the staging.current tree

[Andy Shevchenko]

On Mon, Apr 08, 2019 at 01:01:51PM +0100, Jonathan Cameron wrote:
> On Mon, 8 Apr 2019 13:34:37 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Apr 08, 2019 at 11:14:39AM +0100, Jonathan Cameron wrote:
> > > On Mon, 8 Apr 2019 13:01:21 +0300
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:  
> > > > On Mon, Apr 08, 2019 at 09:14:58AM +0100, Jonathan Cameron wrote:  
> > > > > On Mon, 8 Apr 2019 13:02:12 +1000
> > > > > Stephen Rothwell <sfr@canb.auug.org.au> wrote:  

> > > > > That is the correct resolution.    
> > > > 
> > > > I think it still misses the following fix:  
> > 
> > > Is that actually a problem given it's copied over from buffer->scan_mask just after allocation?
> > > The two masks are the same length so I don't think we have a problem with this one.
> > > Am I missing something?  
> > 
> > Hmm... I didn't get why the commit 20ea39ef9f2f fixes anything.
> > 
> Good point.  I'm don't think it ever did.  
> 
> Alex, any thoughts?

I have a thought that it might be possible that somewhere code is still broken,
i.e. accessing bitmap behind the size (for example, iterating by unsigned long
without bitmap size being aligned to size of unsigned long).

If this is a case, the mentioned patch has a symptomatic healing and not fixing
a root cause.

-- 
With Best Regards,
Andy Shevchenko

Re: linux-next: manual merge of the staging tree with the staging.current tree

[Alexandru Ardelean]

On Tue, Apr 9, 2019 at 6:40 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Apr 08, 2019 at 01:01:51PM +0100, Jonathan Cameron wrote:
> > On Mon, 8 Apr 2019 13:34:37 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Apr 08, 2019 at 11:14:39AM +0100, Jonathan Cameron wrote:
> > > > On Mon, 8 Apr 2019 13:01:21 +0300
> > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Mon, Apr 08, 2019 at 09:14:58AM +0100, Jonathan Cameron wrote:
> > > > > > On Mon, 8 Apr 2019 13:02:12 +1000
> > > > > > Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> > > > > > That is the correct resolution.
> > > > >
> > > > > I think it still misses the following fix:
> > >
> > > > Is that actually a problem given it's copied over from buffer->scan_mask just after allocation?
> > > > The two masks are the same length so I don't think we have a problem with this one.
> > > > Am I missing something?
> > >
> > > Hmm... I didn't get why the commit 20ea39ef9f2f fixes anything.
> > >
> > Good point.  I'm don't think it ever did.
> >
> > Alex, any thoughts?
>

Hey,

This seems to have been in the context of our tree.
We have this patch:
https://github.com/analogdevicesinc/linux/commit/81d00795b1537

That removes bitmap_copy() .
See here:
https://github.com/analogdevicesinc/linux/commit/81d00795b1537#diff-0a87744ce945d2c1c89ea19f21fb35bbL397

This change is not upstreamed yet.
I guess I am slowly going nuts with trying to sync multiple trees [
our master, upstream IIO & some internal temp-branches ].

To give a bit of background: we've noticed this weird behavior while
testing a AD7193 chip with the AD7192 driver and some weird things
were happening.
At the time, this patch seemed easy to send upstream, so I sent it.

Sorry for the noise.

I guess the conclusion is, that in the context of the mainline IIO
tree, commit 20ea39ef9f2f is not needed.

Thanks
Alex

> I have a thought that it might be possible that somewhere code is still broken,
> i.e. accessing bitmap behind the size (for example, iterating by unsigned long
> without bitmap size being aligned to size of unsigned long).
>
> If this is a case, the mentioned patch has a symptomatic healing and not fixing
> a root cause.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>