linux-kernel - RE: [PATCH 2/3] remoteproc: recover a remoteproc when it has crashed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 22 Aug 2012 13:57:32 +0200
From:	Sjur BRENDELAND <sjur.brandeland@...ricsson.com>
To:	"Guzman Lugo, Fernando" <fernando.lugo@...com>
Cc:	Ohad Ben-Cohen <ohad@...ery.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 2/3] remoteproc: recover a remoteproc when it has crashed

Hi Fernando,

> > I think you drop the driver module's ref count during recovery, because
> > rproc_shutdown calls module_put(). Maybe you should move driver
> > ref count handling to rproc_add and rproc_type_release, instead of
> > rproc_boot() and rproc_shutdown()?
> 
> How could you remove the remoteproc modules then?
...
> At this point we have a circular dependency, omap_remoteproc depends on
> remoteproc and vice-versa. Making impossible to remove the remoteproc
> modules. So, I don't think it is a good idea to move module_get/put
> calls.

Yes, I see - you're right. What I proposed won't work.

> However, if you still have concerns about that or a valid testcase where
> that would have an unexpected behaviour we can think in a way to fix it.
> Probably just calling module_get/put inside  rproc_trigger_recover is
> the easiest way. Please let me know what you think.

My concern was unloading of pdev or driver modules durng async firmware
loading. However it turns out that my issue was that I was using
rproc_put() instead of rproc_del() when unloading my driver module.
So after fixing my driver your patch works fine even when unloading the
driver during firmware loading.

> > >+int rproc_trigger_recover(struct rproc *rproc)
> > >+{
> > >+      struct rproc_vdev *rvdev, *rvtmp;
> > >+
> > >+      dev_err(&rproc->dev, "recovering %s\n", rproc->name);
> > >+
> > >+      /* clean up remote vdev entries */
> > >+      list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node)
> > >+              rproc_remove_virtio_dev(rvdev);
> > ...
> > Is this safe? Are you guaranteed that rproc->power
> > is counted down to zero at this point.
...
> > I think you should wait for the rproc->power count to go to zero
> > before initiating the firmware_loading. You could e.g.
> > move firmware_loading to rproc_shutdown(), or add a
> > completion.
> 
> Yes, that sounds good and it will work when we supports
> rproc_boot/shutdown > calls from anywhere and not only from
> rpmsg module(find_vqs). Also, I think with that wait_for_completion
> inside rproc_trigger_recover will make more clear how the recovery
> is performed (actually the need for ref counter to
> become 0 in order to work).
> 
> I will try that and send a new patch to get more comments.


Ok, looking forward to your next patch then.

Regards,
Sjur
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/