Re: [PATCH] usb: gadget: f_fs: add "zombie" mode

[Felipe Balbi <balbi@ti.com>]

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

Hi,

On Tue, Oct 07, 2014 at 06:37:26PM +0200, Krzysztof Opasiak wrote:

[snip]

> > yeah, and the way to deal with that is disconnecting from the host
> > because that USB function, can't be functional anymore. I mean,
> > imagine you try to e.g. unload pictures from your nice DSLR and
> > that DSLR runs Linux and implements MTP or PTP using FFS. Then ptpd
> > dies and you're still connected to the host so you can't know that
> > something went wrong, the camera just stoped sending you data. So
> > you figure: well, it must just be slow, I'll leave it here and go
> > have a nap. Hours later and nothing has changed, because ptpd is
> > still missing.
> > 
> > If you disconnect from the host, however, user knows
> > instantaneously that something went wrong.
> 
> Please believe me that I totally agree with you, but I also see Robert's
> point. Let's take ADB as example. Before ADB has been ported to
> FunctionFS it communicated with kernel using dev node provided by ADB
> function driver. With that infrastructure death of daemon didn't cause
> gadget unbind because kernel driver of that function was just stalling
> the endpoints. This allows user to use all other functions of this

I really mixed feelings about that. It's really counter-intuitive to
allow a non-working function to be exposed to the host.

> gadget. In current design ADB uses FunctionFS and for example if daemon
> will be killed by OOM whole gadget get's unbind and user cannot use any
> other function. Don't you think that's a bit of regression?

Why ? Why would you event want to keep the other functions working ?
Since you're running out of memory anyway, you'd just delay the
inevitable. Soon enough you won't be able to transfer files through
mtp/ptp, or enable usb tethering, or any of that.

> I see and understand yours and Roberts point of view. Personally I'm not
> too enthusiastic about using this solution but I see some rationales for
> this and use cases. Summing it up, this patch may be useful in some
> case. Let's allow end user to decide whether to use this mode or not. I
> think that a few people will find this useful.

It can't be only end user, we have to also consider USB certification.
If we go to certification with a non-working function (say adbd crashed
during init or whatever), then we won't pass certification.

I would rather cause the gadget to disconnect so any crashes on adbd can
be fixed and we pass certification, then exposing that non-working
function to the host.

OOM is another situation which we don't really have control over.
There's nothing we can do to prevent an application from malloc(1 << 30)
and cause OOM to go crazy, however arguably that application is wrong
for allocating 1GiB of memory, in any case, you get the point :-)

> > I don't think maintaining a "zombie" function is very nice. In
> > fact, the very reason for adding usb_function_activate/deactivate
> > was exactly to prevent us from ever connecting to a host with a
> > non-working function.
> 
> Here also I agree. Zombie mode should "mock" the function until first
> next enumeration or unbind. It should not be possible to bind gadget
> with function in zombie mode to UDC. Zombie mode should "pretend" only
> as long as gadget is bound and enumerated.

Not really. We shouldn't even coonect to host until adbd is running.
Now, when adbd crashes we fix adbd. If it gets killed due to OOM we
can't even say "ok, we'll buffer USB requests until adbd is restarted"
because, well, we're running out of memory.

So, OOM we can't fix. Soon enough, another daemon (mtpd, ptpd, whatever)
will be killed and another function will be left unusable.

As for adbd/mtpd/ptpd crashes, those need to fixed and kernel should not
have to deal with them in any way.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]