All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg Edwards <gedwards@ddn.com>
To: intel-wired-lan@osuosl.org
Subject: [Intel-wired-lan] [PATCH 3/3] igb: do not drop PF mailbox lock after read of VF message
Date: Mon, 17 Jul 2017 17:06:31 -0600	[thread overview]
Message-ID: <20170717230631.GA24395@psuche> (raw)
In-Reply-To: <CAKgT0UdRfaxcy6FodbJojENpUA+Fi7YY39EbKvm5PUQB_G_52Q@mail.gmail.com>

On Fri, Jul 14, 2017 at 09:03:50PM -0700, Alexander Duyck wrote:
> On Wed, Jun 28, 2017 at 8:22 AM, Greg Edwards <gedwards@ddn.com> wrote:
>> When the PF receives a mailbox message from the VF, it grabs the mailbox
>> lock, reads the VF message from the mailbox, ACKs the message and drops
>> the lock.
>>
>> While the PF is performing the action for the VF message, nothing
>> prevents another VF message from being posted to the mailbox.  The
>> current code handles this condition by just dropping any new VF messages
>> without processing them.  This results in a mailbox timeout in the VM
>> for posted messages waiting for an ACK, and the VF is reset by the
>> igbvf_watchdog_task in the VM.
>>
>> Given the right sequence of VF messages and mailbox timeouts, this
>> condition can go on ad infinitum.
>>
>> Modify the PF mailbox read method to take an 'unlock' argument that
>> optionally leaves the mailbox locked by the PF after reading the VF
>> message.  This ensures another VF message is not posted to the mailbox
>> until after the PF has completed processing the VF message and written
>> its reply.
>
> I really don't like the design of this patch. It is fixing the PF for
> a VF bug. The VF should not be sending multiple messages to the PF at
> the same time. In the case of the Linux VF anyway there should be the
> RTNL to prevent multiple messages from being sent. If we are sending
> multiple messages at the same time from the VF it points at a bug in
> the VF, not the PF driver.

Sure, I can look at fixing it in the VF driver, if that would be the
preferred approach.  The I350 datasheet made it sound like the PF should
be able to handle this condition (Section 7.8.2.9.1 "VF to PF Mailbox"),
but I realize that may be implementation specific.

Greg

      reply	other threads:[~2017-07-17 23:06 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-28 15:22 [Intel-wired-lan] [PATCH 0/3] igb: VF mailbox timeouts with multiple VF messages Greg Edwards
2017-06-28 15:22 ` [Intel-wired-lan] [PATCH 1/3] igb: add argument names to mailbox op function declarations Greg Edwards
2017-07-14 21:49   ` Brown, Aaron F
2017-06-28 15:22 ` [Intel-wired-lan] [PATCH 2/3] igb: expose mailbox unlock method Greg Edwards
2017-07-14 21:49   ` Brown, Aaron F
2017-06-28 15:22 ` [Intel-wired-lan] [PATCH 3/3] igb: do not drop PF mailbox lock after read of VF message Greg Edwards
2017-07-14 21:50   ` Brown, Aaron F
2017-07-15  4:03   ` Alexander Duyck
2017-07-17 23:06     ` Greg Edwards [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170717230631.GA24395@psuche \
    --to=gedwards@ddn.com \
    --cc=intel-wired-lan@osuosl.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.