re: spi: Add call to spi_slave_abort() function when spidev driver is released

re: spi: Add call to spi_slave_abort() function when spidev driver is released

[Colin Ian King]

Hi,

Static analysis with Coverity has detected an potential dereference of a
free'd object with commit:

commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
Author: Lukasz Majewski <lukma@denx.de>
Date:   Wed Sep 25 11:11:42 2019 +0200

    spi: Add call to spi_slave_abort() function when spidev driver is
released

In spidev_release() in drivers/spi/spidev.c the analysis is as follows:

600static int spidev_release(struct inode *inode, struct file *filp)
601{
602        struct spidev_data      *spidev;
603
604        mutex_lock(&device_list_lock);

   1. alias: Assigning: spidev = filp->private_data. Now both point to
the same storage.

605        spidev = filp->private_data;
606        filp->private_data = NULL;
607
608        /* last close? */
609        spidev->users--;

   2. Condition !spidev->users, taking true branch.

610        if (!spidev->users) {
611                int             dofree;
612
613                kfree(spidev->tx_buffer);
614                spidev->tx_buffer = NULL;
615
616                kfree(spidev->rx_buffer);
617                spidev->rx_buffer = NULL;
618
619                spin_lock_irq(&spidev->spi_lock);

   3. Condition spidev->spi, taking false branch.

620                if (spidev->spi)
621                        spidev->speed_hz = spidev->spi->max_speed_hz;
622
623                /* ... after we unbound from the underlying device? */

   4. Condition spidev->spi == NULL, taking true branch.

624                dofree = (spidev->spi == NULL);
625                spin_unlock_irq(&spidev->spi_lock);
626

   5. Condition dofree, taking true branch.

627                if (dofree)

   6. freed_arg: kfree frees spidev.

628                        kfree(spidev);
629        }
630#ifdef CONFIG_SPI_SLAVE

   CID 89726 (#1 of 1): Read from pointer after free (USE_AFTER_FREE)
7. deref_after_free: Dereferencing freed pointer spidev.

631        spi_slave_abort(spidev->spi);
632#endif
633        mutex_unlock(&device_list_lock);
634
635        return 0;
636}

The call to spi_slave_abort() on spidev is reading an earlier kfree'd
spidev.

Colin

Re: spi: Add call to spi_slave_abort() function when spidev driver is released

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2866 bytes --]

Hi Colin,

> Hi,
> 
> Static analysis with Coverity has detected an potential dereference
> of a free'd object with commit:
> 
> commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
> Author: Lukasz Majewski <lukma@denx.de>
> Date:   Wed Sep 25 11:11:42 2019 +0200
> 
>     spi: Add call to spi_slave_abort() function when spidev driver is
> released
> 
> In spidev_release() in drivers/spi/spidev.c the analysis is as
> follows:
> 
> 600static int spidev_release(struct inode *inode, struct file *filp)
> 601{
> 602        struct spidev_data      *spidev;
> 603
> 604        mutex_lock(&device_list_lock);
> 
>    1. alias: Assigning: spidev = filp->private_data. Now both point to
> the same storage.
> 
> 605        spidev = filp->private_data;
> 606        filp->private_data = NULL;
> 607
> 608        /* last close? */
> 609        spidev->users--;
> 
>    2. Condition !spidev->users, taking true branch.
> 
> 610        if (!spidev->users) {
> 611                int             dofree;
> 612
> 613                kfree(spidev->tx_buffer);
> 614                spidev->tx_buffer = NULL;
> 615
> 616                kfree(spidev->rx_buffer);
> 617                spidev->rx_buffer = NULL;
> 618
> 619                spin_lock_irq(&spidev->spi_lock);
> 
>    3. Condition spidev->spi, taking false branch.
> 
> 620                if (spidev->spi)
> 621                        spidev->speed_hz =
> spidev->spi->max_speed_hz; 622
> 623                /* ... after we unbound from the underlying
> device? */
> 
>    4. Condition spidev->spi == NULL, taking true branch.
> 
> 624                dofree = (spidev->spi == NULL);
> 625                spin_unlock_irq(&spidev->spi_lock);
> 626
> 
>    5. Condition dofree, taking true branch.
> 
> 627                if (dofree)
> 
>    6. freed_arg: kfree frees spidev.
> 
> 628                        kfree(spidev);
> 629        }
> 630#ifdef CONFIG_SPI_SLAVE
> 
>    CID 89726 (#1 of 1): Read from pointer after free (USE_AFTER_FREE)
> 7. deref_after_free: Dereferencing freed pointer spidev.
> 
> 631        spi_slave_abort(spidev->spi);
> 632#endif
> 633        mutex_unlock(&device_list_lock);
> 634
> 635        return 0;
> 636}
> 
> The call to spi_slave_abort() on spidev is reading an earlier kfree'd
> spidev.

Thanks for spotting this issue - indeed there is a possibility to use
spidev after being kfree'd. 

However, Geert (CC'ed) had some questions about placement of this
function call, so I will wait with providing fix until he replies.

> 
> Colin
> 


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: spi: Add call to spi_slave_abort() function when spidev driver is released

[Colin Ian King]

[-- Attachment #1.1: Type: text/plain, Size: 3060 bytes --]

On 26/09/2019 11:14, Lukasz Majewski wrote:
> Hi Colin,
> 
>> Hi,
>>
>> Static analysis with Coverity has detected an potential dereference
>> of a free'd object with commit:
>>
>> commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
>> Author: Lukasz Majewski <lukma@denx.de>
>> Date:   Wed Sep 25 11:11:42 2019 +0200
>>
>>     spi: Add call to spi_slave_abort() function when spidev driver is
>> released
>>
>> In spidev_release() in drivers/spi/spidev.c the analysis is as
>> follows:
>>
>> 600static int spidev_release(struct inode *inode, struct file *filp)
>> 601{
>> 602        struct spidev_data      *spidev;
>> 603
>> 604        mutex_lock(&device_list_lock);
>>
>>    1. alias: Assigning: spidev = filp->private_data. Now both point to
>> the same storage.
>>
>> 605        spidev = filp->private_data;
>> 606        filp->private_data = NULL;
>> 607
>> 608        /* last close? */
>> 609        spidev->users--;
>>
>>    2. Condition !spidev->users, taking true branch.
>>
>> 610        if (!spidev->users) {
>> 611                int             dofree;
>> 612
>> 613                kfree(spidev->tx_buffer);
>> 614                spidev->tx_buffer = NULL;
>> 615
>> 616                kfree(spidev->rx_buffer);
>> 617                spidev->rx_buffer = NULL;
>> 618
>> 619                spin_lock_irq(&spidev->spi_lock);
>>
>>    3. Condition spidev->spi, taking false branch.
>>
>> 620                if (spidev->spi)
>> 621                        spidev->speed_hz =
>> spidev->spi->max_speed_hz; 622
>> 623                /* ... after we unbound from the underlying
>> device? */
>>
>>    4. Condition spidev->spi == NULL, taking true branch.
>>
>> 624                dofree = (spidev->spi == NULL);
>> 625                spin_unlock_irq(&spidev->spi_lock);
>> 626
>>
>>    5. Condition dofree, taking true branch.
>>
>> 627                if (dofree)
>>
>>    6. freed_arg: kfree frees spidev.
>>
>> 628                        kfree(spidev);
>> 629        }
>> 630#ifdef CONFIG_SPI_SLAVE
>>
>>    CID 89726 (#1 of 1): Read from pointer after free (USE_AFTER_FREE)
>> 7. deref_after_free: Dereferencing freed pointer spidev.
>>
>> 631        spi_slave_abort(spidev->spi);
>> 632#endif
>> 633        mutex_unlock(&device_list_lock);
>> 634
>> 635        return 0;
>> 636}
>>
>> The call to spi_slave_abort() on spidev is reading an earlier kfree'd
>> spidev.
> 
> Thanks for spotting this issue - indeed there is a possibility to use
> spidev after being kfree'd. 
> 
> However, Geert (CC'ed) had some questions about placement of this
> function call, so I will wait with providing fix until he replies.

Cool, thanks for the update.

Colin
> 
>>
>> Colin
>>
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: spi: Add call to spi_slave_abort() function when spidev driver is released

[Geert Uytterhoeven]

Hi Lukasz,

On Thu, Sep 26, 2019 at 12:14 PM Lukasz Majewski <lukma@denx.de> wrote:
> > Static analysis with Coverity has detected an potential dereference
> > of a free'd object with commit:
> >
> > commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
> > Author: Lukasz Majewski <lukma@denx.de>
> > Date:   Wed Sep 25 11:11:42 2019 +0200
> >
> >     spi: Add call to spi_slave_abort() function when spidev driver is
> > released
> >
> > In spidev_release() in drivers/spi/spidev.c the analysis is as
> > follows:
> >
> > 600static int spidev_release(struct inode *inode, struct file *filp)
> > 601{
> > 602        struct spidev_data      *spidev;
> > 603
> > 604        mutex_lock(&device_list_lock);
> >
> >    1. alias: Assigning: spidev = filp->private_data. Now both point to
> > the same storage.
> >
> > 605        spidev = filp->private_data;
> > 606        filp->private_data = NULL;
> > 607
> > 608        /* last close? */
> > 609        spidev->users--;
> >
> >    2. Condition !spidev->users, taking true branch.
> >
> > 610        if (!spidev->users) {
> > 611                int             dofree;
> > 612
> > 613                kfree(spidev->tx_buffer);
> > 614                spidev->tx_buffer = NULL;
> > 615
> > 616                kfree(spidev->rx_buffer);
> > 617                spidev->rx_buffer = NULL;
> > 618
> > 619                spin_lock_irq(&spidev->spi_lock);
> >
> >    3. Condition spidev->spi, taking false branch.
> >
> > 620                if (spidev->spi)
> > 621                        spidev->speed_hz =
> > spidev->spi->max_speed_hz; 622
> > 623                /* ... after we unbound from the underlying
> > device? */
> >
> >    4. Condition spidev->spi == NULL, taking true branch.
> >
> > 624                dofree = (spidev->spi == NULL);
> > 625                spin_unlock_irq(&spidev->spi_lock);
> > 626
> >
> >    5. Condition dofree, taking true branch.
> >
> > 627                if (dofree)
> >
> >    6. freed_arg: kfree frees spidev.
> >
> > 628                        kfree(spidev);
> > 629        }
> > 630#ifdef CONFIG_SPI_SLAVE
> >
> >    CID 89726 (#1 of 1): Read from pointer after free (USE_AFTER_FREE)
> > 7. deref_after_free: Dereferencing freed pointer spidev.
> >
> > 631        spi_slave_abort(spidev->spi);
> > 632#endif
> > 633        mutex_unlock(&device_list_lock);
> > 634
> > 635        return 0;
> > 636}
> >
> > The call to spi_slave_abort() on spidev is reading an earlier kfree'd
> > spidev.
>
> Thanks for spotting this issue - indeed there is a possibility to use
> spidev after being kfree'd.

Worse, this makes me realize spidev->spi may be a NULL pointer, which
will be dereferenced by spi_slave_abort(), so caching it before the
call to kfree() won't work.

> However, Geert (CC'ed) had some questions about placement of this
> function call, so I will wait with providing fix until he replies.

Seems like this needs more thought...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: spi: Add call to spi_slave_abort() function when spidev driver is released

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 4628 bytes --]

Hi Geert,

> Hi Lukasz,
> 
> On Thu, Sep 26, 2019 at 12:14 PM Lukasz Majewski <lukma@denx.de>
> wrote:
> > > Static analysis with Coverity has detected an potential
> > > dereference of a free'd object with commit:
> > >
> > > commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
> > > Author: Lukasz Majewski <lukma@denx.de>
> > > Date:   Wed Sep 25 11:11:42 2019 +0200
> > >
> > >     spi: Add call to spi_slave_abort() function when spidev
> > > driver is released
> > >
> > > In spidev_release() in drivers/spi/spidev.c the analysis is as
> > > follows:
> > >
> > > 600static int spidev_release(struct inode *inode, struct file
> > > *filp) 601{
> > > 602        struct spidev_data      *spidev;
> > > 603
> > > 604        mutex_lock(&device_list_lock);
> > >
> > >    1. alias: Assigning: spidev = filp->private_data. Now both
> > > point to the same storage.
> > >
> > > 605        spidev = filp->private_data;
> > > 606        filp->private_data = NULL;
> > > 607
> > > 608        /* last close? */
> > > 609        spidev->users--;
> > >
> > >    2. Condition !spidev->users, taking true branch.
> > >
> > > 610        if (!spidev->users) {
> > > 611                int             dofree;
> > > 612
> > > 613                kfree(spidev->tx_buffer);
> > > 614                spidev->tx_buffer = NULL;
> > > 615
> > > 616                kfree(spidev->rx_buffer);
> > > 617                spidev->rx_buffer = NULL;
> > > 618
> > > 619                spin_lock_irq(&spidev->spi_lock);
> > >
> > >    3. Condition spidev->spi, taking false branch.
> > >
> > > 620                if (spidev->spi)
> > > 621                        spidev->speed_hz =
> > > spidev->spi->max_speed_hz; 622
> > > 623                /* ... after we unbound from the underlying
> > > device? */
> > >
> > >    4. Condition spidev->spi == NULL, taking true branch.
> > >
> > > 624                dofree = (spidev->spi == NULL);
> > > 625                spin_unlock_irq(&spidev->spi_lock);
> > > 626
> > >
> > >    5. Condition dofree, taking true branch.
> > >
> > > 627                if (dofree)
> > >
> > >    6. freed_arg: kfree frees spidev.
> > >
> > > 628                        kfree(spidev);
> > > 629        }
> > > 630#ifdef CONFIG_SPI_SLAVE
> > >
> > >    CID 89726 (#1 of 1): Read from pointer after free
> > > (USE_AFTER_FREE) 7. deref_after_free: Dereferencing freed pointer
> > > spidev.
> > >
> > > 631        spi_slave_abort(spidev->spi);
> > > 632#endif
> > > 633        mutex_unlock(&device_list_lock);
> > > 634
> > > 635        return 0;
> > > 636}
> > >
> > > The call to spi_slave_abort() on spidev is reading an earlier
> > > kfree'd spidev.  
> >
> > Thanks for spotting this issue - indeed there is a possibility to
> > use spidev after being kfree'd.  
> 
> Worse, this makes me realize spidev->spi may be a NULL pointer, which
> will be dereferenced by spi_slave_abort(), so caching it before the
> call to kfree() won't work.
> 

The patch as it is now can be fixed as follows:

static int spidev_release(struct inode *inode, struct file *filp)
{
	struct spidev_data	*spidev;

	mutex_lock(&device_list_lock);
	spidev = filp->private_data;
	filp->private_data = NULL;

#ifdef CONFIG_SPI_SLAVE
	if (spidev->spi)
		spi_slave_abort(spidev->spi);
#endif

	/* last close? */
	spidev->users--;
	if (!spidev->users) {
		int dofree;

		/* free buffers */

		spin_lock_irq(&spidev->spi_lock);
		if (spidev->spi)
			spidev->speed_hz = spidev->spi->max_speed_hz;

		/* ... after we unbound from the underlying device? */
		//
		// [*]
		//
		dofree = (spidev->spi == NULL);
		spin_unlock_irq(&spidev->spi_lock);

		if (dofree)
			kfree(spidev);
	}

	mutex_unlock(&device_list_lock);

	return 0;
}

The question is if we shall call the spi_slave_abort() when cleaning up
spi after releasing last reference, or each time release callback is
called ?

> > However, Geert (CC'ed) had some questions about placement of this
> > function call, so I will wait with providing fix until he replies.  
> 
> Seems like this needs more thought...

Could you be more specific? 

Do you mean to move the spi_slave_abort() call just before dofree
evaluation ? ([*]).

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: spi: Add call to spi_slave_abort() function when spidev driver is released

[Geert Uytterhoeven]

Hi Lukasz,

On Thu, Sep 26, 2019 at 2:49 PM Lukasz Majewski <lukma@denx.de> wrote:
> > On Thu, Sep 26, 2019 at 12:14 PM Lukasz Majewski <lukma@denx.de>
> > wrote:
> > > > Static analysis with Coverity has detected an potential
> > > > dereference of a free'd object with commit:
> > > >
> > > > commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
> > > > Author: Lukasz Majewski <lukma@denx.de>
> > > > Date:   Wed Sep 25 11:11:42 2019 +0200
> > > >
> > > >     spi: Add call to spi_slave_abort() function when spidev
> > > > driver is released

> > > > The call to spi_slave_abort() on spidev is reading an earlier
> > > > kfree'd spidev.
> > >
> > > Thanks for spotting this issue - indeed there is a possibility to
> > > use spidev after being kfree'd.
> >
> > Worse, this makes me realize spidev->spi may be a NULL pointer, which
> > will be dereferenced by spi_slave_abort(), so caching it before the
> > call to kfree() won't work.
>
> The patch as it is now can be fixed as follows:
>
> static int spidev_release(struct inode *inode, struct file *filp)
> {
>         struct spidev_data      *spidev;
>
>         mutex_lock(&device_list_lock);
>         spidev = filp->private_data;
>         filp->private_data = NULL;
>
> #ifdef CONFIG_SPI_SLAVE
>         if (spidev->spi)
>                 spi_slave_abort(spidev->spi);
> #endif
>
>         /* last close? */
>         spidev->users--;
>         if (!spidev->users) {
>                 int dofree;
>
>                 /* free buffers */
>
>                 spin_lock_irq(&spidev->spi_lock);
>                 if (spidev->spi)
>                         spidev->speed_hz = spidev->spi->max_speed_hz;
>
>                 /* ... after we unbound from the underlying device? */
>                 //
>                 // [*]
>                 //
>                 dofree = (spidev->spi == NULL);
>                 spin_unlock_irq(&spidev->spi_lock);
>
>                 if (dofree)
>                         kfree(spidev);
>         }
>
>         mutex_unlock(&device_list_lock);
>
>         return 0;
> }
>
> The question is if we shall call the spi_slave_abort() when cleaning up
> spi after releasing last reference, or each time release callback is
> called ?

TBH, I don't know.  Is it realistic that there are multiple opens?

> > > However, Geert (CC'ed) had some questions about placement of this
> > > function call, so I will wait with providing fix until he replies.
> >
> > Seems like this needs more thought...
>
> Could you be more specific?
>
> Do you mean to move the spi_slave_abort() call just before dofree
> evaluation ? ([*]).

That means the abort is called only for the last user.
And only if the underlying device still exists.  Which means that if it has
disappeared (how can that happen? spidev unbind?), the slave was never
aborted.  Non-spidev slaves can do the abort in their .remove() callbacks
(at least my two sample slave drivers do).
So probably we need some explicit slave abort in the unbind case too?

The more I think about it, the more things I see that can go wrong...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: spi: Add call to spi_slave_abort() function when spidev driver is released

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 4400 bytes --]

Hi Geert,

> Hi Lukasz,
> 
> On Thu, Sep 26, 2019 at 2:49 PM Lukasz Majewski <lukma@denx.de> wrote:
> > > On Thu, Sep 26, 2019 at 12:14 PM Lukasz Majewski <lukma@denx.de>
> > > wrote:  
> > > > > Static analysis with Coverity has detected an potential
> > > > > dereference of a free'd object with commit:
> > > > >
> > > > > commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
> > > > > Author: Lukasz Majewski <lukma@denx.de>
> > > > > Date:   Wed Sep 25 11:11:42 2019 +0200
> > > > >
> > > > >     spi: Add call to spi_slave_abort() function when spidev
> > > > > driver is released  
> 
> > > > > The call to spi_slave_abort() on spidev is reading an earlier
> > > > > kfree'd spidev.  
> > > >
> > > > Thanks for spotting this issue - indeed there is a possibility
> > > > to use spidev after being kfree'd.  
> > >
> > > Worse, this makes me realize spidev->spi may be a NULL pointer,
> > > which will be dereferenced by spi_slave_abort(), so caching it
> > > before the call to kfree() won't work.  
> >
> > The patch as it is now can be fixed as follows:
> >
> > static int spidev_release(struct inode *inode, struct file *filp)
> > {
> >         struct spidev_data      *spidev;
> >
> >         mutex_lock(&device_list_lock);
> >         spidev = filp->private_data;
> >         filp->private_data = NULL;
> >
> > #ifdef CONFIG_SPI_SLAVE
> >         if (spidev->spi)
> >                 spi_slave_abort(spidev->spi);
> > #endif
> >
> >         /* last close? */
> >         spidev->users--;
> >         if (!spidev->users) {
> >                 int dofree;
> >
> >                 /* free buffers */
> >
> >                 spin_lock_irq(&spidev->spi_lock);
> >                 if (spidev->spi)
> >                         spidev->speed_hz =
> > spidev->spi->max_speed_hz;
> >
> >                 /* ... after we unbound from the underlying device?
> > */ //
> >                 // [*]
> >                 //
> >                 dofree = (spidev->spi == NULL);
> >                 spin_unlock_irq(&spidev->spi_lock);
> >
> >                 if (dofree)
> >                         kfree(spidev);
> >         }
> >
> >         mutex_unlock(&device_list_lock);
> >
> >         return 0;
> > }
> >
> > The question is if we shall call the spi_slave_abort() when
> > cleaning up spi after releasing last reference, or each time
> > release callback is called ?  
> 
> TBH, I don't know.  Is it realistic that there are multiple opens?

I'm using on my setup only one test program to use /dev/spidevX.Y and
/dev/spidevA.B (loopback with wired connection).

However, you also shall be able to connect via ssh and run the same
setup in parallel...

> 
> > > > However, Geert (CC'ed) had some questions about placement of
> > > > this function call, so I will wait with providing fix until he
> > > > replies.  
> > >
> > > Seems like this needs more thought...  
> >
> > Could you be more specific?
> >
> > Do you mean to move the spi_slave_abort() call just before dofree
> > evaluation ? ([*]).  
> 
> That means the abort is called only for the last user.
> And only if the underlying device still exists.  Which means that if
> it has disappeared (how can that happen? spidev unbind?),

In my case, I just disconnect some SPI signals and the test program
just hangs. I do need to ctrl+c to stop it (or use timeout). 

From my debugging the .release callback is called each time the program
is aborted (either with ctrl+c or timeout).

> the slave
> was never aborted.  Non-spidev slaves can do the abort in their
> .remove() callbacks (at least my two sample slave drivers do).
> So probably we need some explicit slave abort in the unbind case too?

As I've described above - after "introducing" distortion to SPI I need
to explicitly exit the hung test program with ctrl+c.

> 
> The more I think about it, the more things I see that can go wrong...

But for now we don't have any way to recover the slave after corruption
on SPI transmission.

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: spi: Add call to spi_slave_abort() function when spidev driver is released

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

On Thu, Sep 26, 2019 at 04:06:45PM +0200, Lukasz Majewski wrote:
> > On Thu, Sep 26, 2019 at 2:49 PM Lukasz Majewski <lukma@denx.de> wrote:

> > > The question is if we shall call the spi_slave_abort() when
> > > cleaning up spi after releasing last reference, or each time
> > > release callback is called ?  

> > TBH, I don't know.  Is it realistic that there are multiple opens?

> I'm using on my setup only one test program to use /dev/spidevX.Y and
> /dev/spidevA.B (loopback with wired connection).

> However, you also shall be able to connect via ssh and run the same
> setup in parallel...

It doesn't seem entirely realistic, but I can imagine cases like
fork()/exec() where we end up with two copies of the file open
but end up immediately closing one.

> > That means the abort is called only for the last user.
> > And only if the underlying device still exists.  Which means that if
> > it has disappeared (how can that happen? spidev unbind?),

> In my case, I just disconnect some SPI signals and the test program
> just hangs. I do need to ctrl+c to stop it (or use timeout). 

> From my debugging the .release callback is called each time the program
> is aborted (either with ctrl+c or timeout).

Should be on file close IIRC.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: spi: Add call to spi_slave_abort() function when spidev driver is released

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 1831 bytes --]

Hi Mark, Geert,

> On Thu, Sep 26, 2019 at 04:06:45PM +0200, Lukasz Majewski wrote:
> > > On Thu, Sep 26, 2019 at 2:49 PM Lukasz Majewski <lukma@denx.de>
> > > wrote:  
> 
> > > > The question is if we shall call the spi_slave_abort() when
> > > > cleaning up spi after releasing last reference, or each time
> > > > release callback is called ?    
> 
> > > TBH, I don't know.  Is it realistic that there are multiple
> > > opens?  
> 
> > I'm using on my setup only one test program to use /dev/spidevX.Y
> > and /dev/spidevA.B (loopback with wired connection).  
> 
> > However, you also shall be able to connect via ssh and run the same
> > setup in parallel...  
> 
> It doesn't seem entirely realistic, but I can imagine cases like
> fork()/exec() where we end up with two copies of the file open
> but end up immediately closing one.
> 
> > > That means the abort is called only for the last user.
> > > And only if the underlying device still exists.  Which means that
> > > if it has disappeared (how can that happen? spidev unbind?),  
> 
> > In my case, I just disconnect some SPI signals and the test program
> > just hangs. I do need to ctrl+c to stop it (or use timeout).   
> 
> > From my debugging the .release callback is called each time the
> > program is aborted (either with ctrl+c or timeout).  
> 
> Should be on file close IIRC.

Any ideas on how to solve this issue?

Maybe, it would be sufficient for now to move the spi_slave_abort() in
spi_release() before we decrease (spidev->users--) the use count?


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: spi: Add call to spi_slave_abort() function when spidev driver is released

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

On Fri, Sep 27, 2019 at 12:38:49AM +0200, Lukasz Majewski wrote:

> Maybe, it would be sufficient for now to move the spi_slave_abort() in
> spi_release() before we decrease (spidev->users--) the use count?

I think that should be OK, or possibly safer to do it at the start of
the if (!spidev->users) section to avoid problems with fork()/exec()
combinations.  It'll need an if (spidev->spi) check as well, if we
manage to hit that we've pretty much lost anyway though and I'm not sure
I see a route to sensible error handling.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]