Hi Arend, Franky,
------- Original Message -------
On Tuesday, June 7th, 2022 at 21:45, Danny van Heumen <danny@xxxxxxxxxxxxxxxxx> wrote:
> Hi Arend,
>
> ------- Original Message -------
> On Tuesday, June 7th, 2022 at 21:00, Arend van Spriel arend.vanspriel@xxxxxxxxxxxx wrote:
>
> > [..]
> >
> > > > While directly return without invoking clean up process makes
> > > > perfect sense for the issue described here, it doesn’t address the
> > > > broader issue that sdiodev might hold on to couple stale pointers that
> > > > might subsequently be used in somewhere down the path because sdiodev is
> > > > not freed. Setting these pointer to NULL after freeing them could help
> > > > us to catch such issue which is more catastrophic than a double-free.
> > > > The perfect solution of course is to rework the code to free sdiodev
> > > > whenever brcmf_sdiod_remove() is invoked but that can not be done in
> > > > short-term unfortunately.
> >
> > > - True.
> > > - If the two early returns are appropriate -- I think they are -- so
> > > we can leave those in. (Again, I'm unfamiliar with the code-base.)
> > > - Setting the pointer to NULL at least has the benefit that behavior
> > > (even if bugged) is the same irrespective of memory allocation behavior.
> > > - I checked your suggestion for 'sdiodev->sgtable': it is not a
> >
> > pointer, so setting to NULL will not help. If I read this correctly,
> > 'sg_free_table(..)' is already resistant to multiple freeing attempts
> > with a test of '.sgl'.
> >
> > > .. as for the control flow. Sure, rework would be nice, but -- to me
> > > at least -- it is not clear if it is really necessary. Maybe I'm
> > > mistaken, but there seem to be few entry-points to take into account.
> > > The "hardware-reset after firmware-crash"-logic was added later IIUC, so
> > > maybe it was an oversight? Regardless, I have updated the patch.
> >
> > The reset-after-fw-crash was indeed added later. I am wondering is we
> > can leave the remove-reprobe dance to the mmc core. Maybe mmc_hw_reset()
> > could do the trick, ie. simply call mmc_hw_reset() in
> > brcmf_sdio_bus_reset() and be done with it. Maybe opening a can of worms
> > here, but I always felt uncertain about calling .remove and .probe
> > callbacks directly from the driver itself as it may cause issue like
> > this double-free, but also the bus is unaware and I don't know that is a
> > bad thing or not.
>
>
> I am pretty sure you found a can of worms indeed :-)
>
> So, a few things to note:
>
> - do you have a reliable way to test this behavior?
> - from my PoV: I have encountered various compositions of firmware and uCode
> for the BCM4345/9 (43456-sdio). Earlier versions may occasionally
> exhibit the crashing-behavior, but not reliably.
> - do you want to tackle this as a separate effort, given that there is already
> benefit in merging the earlier patch proposal?
>
> Let me try to give my impression of the code, that I get from my cursory scans
> through the brcmfmac code: it seems that the device as a whole (the "root")
> gets activated. From what I remember, there seems to be a callback that
> triggers subsequent initialization. So, it makes somewhat sense to me if a
> hardware-reset could flow (back) into the existing initialization flow.
> (Again, don't trust info below, as I have very limited knowledge in this domain.
> I'm trying to check the extent to which I can make sense of it.)
What would be the next steps?
I would assume that you are interested in incorporating the bug fixes in some
form. So, I would like to know how we can proceed with this.
Kind regards,
Danny
> > Regards,
> > Arend
> >
> > > > Also I forgot that our IT attached a legal footer to all emails sent
> >
> > to an external party. I am sorry about that to anyone reading my mail
> > but there is nothing I can do at the moment.
> >
> > > > Thanks,
> >
> > > > - Franky
> >
> > > I have attached the updated patch. As mentioned before, I will be
> >
> > running the changes myself.
> >
> > > Regards,
> >
> > > Danny
> >
> > > > > So, returning immediately with the errorcode seemed more
> >
> > appropriate. Regardless, I have only
> >
> > > > > incidental knowledge from checking the code just for this
> >
> > particular problem. In the end my goal
> >
> > > > > is to have the issues addressed so that I am not forced to reboot
> >
> > my system to get it back in
> >
> > > > > working order.
> >
> > > > > As for your remark about sg-table: I had not considered that, but
> >
> > if my notes above check out,
> >
> > > > > maybe this does not need to be treated conditionally at all.
> >
> > > > > Kind regards,
> >
> > > > > Danny
> >
> > > > > > diff --git
> >
> > a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> >
> > > > > > index ac02244a6fdf..e9bad7197ba9 100644
> >
> > > > > > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> >
> > > > > > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> >
> > > > > > @@ -802,6 +802,7 @@ static void brcmf_sdiod_freezer_detach(struct
> >
> > brcmf_sdio_dev *sdiodev)
> >
> > > > > > if (sdiodev->freezer) {
> >
> > > > > > WARN_ON(atomic_read(&sdiodev->freezer->freezing));
> >
> > > > > > kfree(sdiodev->freezer);
> >
> > > > > > + sdiodev->freezer = NULL;
> >
> > > > > > }
> >
> > > > > > }
> >
> > > > > > @@ -885,7 +886,11 @@ int brcmf_sdiod_remove(struct brcmf_sdio_dev
> >
> > *sdiodev)
> >
> > > > > > sdio_disable_func(sdiodev->func1);
> >
> > > > > > sdio_release_host(sdiodev->func1);
> >
> > > > > > - sg_free_table(&sdiodev->sgtable);
> >
> > > > > > + if (sdiodev->sgtable) {
> >
> > > > > > + sg_free_table(&sdiodev->sgtable);
> >
> > > > > > + sdiodev->sgtable = NULL;
> >
> > > > > > + }
> >
> > > > > > +
> >
> > > > > > sdiodev->sbwad = 0;
> >
> > > > > > pm_runtime_allow(sdiodev->func1->card->host->parent);
> >
> > > > > > As for submitting patch to linux-wireless, please follow the
> >
> > guideline. [1]
> >
> > > > > > Thanks,
> >
> > > > > > - Franky
> >
> > > > > > [1]
> >
> > https://www.google.com/url?q=https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches&source=gmail-imap&ust=1654959604000000&usg=AOvVaw1Q6aXVZjiKkrq9qmyYVVDa
> >
> > > > --
> >
> > > > This electronic communication and the information and any files
> >
> > transmitted
> >
> > > > with it, or attached to it, are confidential and are intended solely for
> >
> > > > the use of the individual or entity to whom it is addressed and may
> >
> > contain
> >
> > > > information that is confidential, legally privileged, protected by
> >
> > privacy
> >
> > > > laws, or otherwise restricted from disclosure to anyone else. If you are
> >
> > > > not the intended recipient or the person responsible for delivering the
> >
> > > > e-mail to the intended recipient, you are hereby notified that any use,
> >
> > > > copying, distributing, dissemination, forwarding, printing, or
> >
> > copying of
> >
> > > > this e-mail is strictly prohibited. If you received this e-mail in
> >
> > error,
> >
> > > > please return the e-mail to the sender, delete it from your
> >
> > computer, and
> >
> > > > destroy any printed copy of it.
