Hi Adam,

quoted

Nice!

Yeah.  Had a bit of an internal discussion about this one. Ideally, we 
would stop the queue one packet earlier, and not drop anything.  The 
mbox_send_message only returns the index of the next slot in the ring
buffer and since it is circular, that does not give us a sense  of the 
space left.

I think that's okay as-is. If you have access to the tail pointer of the
ring buffer too, you may be able to calculate space, but that's
1) optional, and 2) best left to a later change.

quoted

quoted

+static int mctp_pcc_ndo_stop(struct net_device *ndev)
+{
+       struct mctp_pcc_ndev *mctp_pcc_ndev =
+           netdev_priv(ndev);

Minor: Unneeded wrapping here, and it seems to be suppressing the
warning about a blank line after declarations.

The Reverse XMasstree format checker I am using seems overly strict.  I 
will try to unwrap all of these.  Is it better to do a separate variable 
initialization?  Seems a bad coding practice for just a format decision 
that is purely aesthetic. But this one is simple to fix.

That shouldn't be tripping any RCT checks here, as it's just one
variable init?

	mctp_pcc_ndev *mctp_pcc_ndev = netdev_priv(ndev);

Keep it in one if possible (as you have done).

quoted

quoted

+       drain_packets(&mctp_pcc_ndev->outbox.packets);
+       drain_packets(&mctp_pcc_ndev->inbox.packets);

Now that you're no longer doing the pcc_mbox_free_channel() in ndo_stop,
nothing has quiesced the pcc channels at this point, right? In which
case you now have a race between the channels' accesses to skb->data and
freeing the skbs here.

(I have written and rewritten this section multiple times, so apoliges 
if soemthing is unclear or awkward...it might reflect and earlier 
thought...)

OK, I think I do need to call pcc_mbox_free_channel here,

You should ensure that the packet processing has stopped on
ndo_stop (and has not started before ndo_open). Without doing that, you
have two issues:

 - the RX packets will continue while the interface is down; and
 - you cannot free the lists

If there's a way to keep the channel allocated, but suspend the
processing of messages, that would seem like a good option (and sounds
like it would solve the MTU complexity).

However, on a cursory look through the pcc/mailbox infrastructure, it
seems like the pcc_mbox_request_channel starts processing immediately -
so it looks like you can not have the channel in an allocated-but-idle
state, since the request_channel does the bind_client, which does a
pcc_startup.

So, I figure you have two options:

 - only request the channel until the interface is up; or

 - implement your own quiescing in the callbacks - keeping the channels
   allocated, but check if the netdev is operational (ie, ndo_open has
   been called) before processing an RX message

which means I need to allocate them in the pairing function. The ring
buffer will still have pointers to the sk_buffs, but they will never
be looked at again: the ring buffer will ger reinitialized if another
client binds to it.

OK, but the skbs will remain allocated between those operations, which
has other side-effects (eg, socket mem accounting). You'll want to drain
the queue (as you are doing) if the queue is no longer making progress.

The removal was to deal with the setting of the MTU, which requires a
channel to read the size of the shared buffer.

        mctp_pcc_mtu = mctp_pcc_ndev->outbox.chan->shmem_size -
                sizeof(struct pcc_header);

I could create a channel, read  the value, and release it.  The Value I 
need is in the ACPI PCC-table but I don't have direct access to it. 
Perhaps it would be better to initialize the value to -1 and use that to 
optionally reset it to the max value on ndo open.

If you cannot create the channel until ndo_open, I'd you will also need
to check the current mtu value on open, and failing if it exceeds the
limit of the channel.

If you have some way to extract this from the ACPI tables, that may be
preferable. With the -1 approach, you'll also need to ensure that the
*current* MTU is not larger than the channel max, on ndo_open. For
example:

  $ mctp link set mctppcc0 mtu 1000
     # sets current mtu, no max currently specified
  $ mctp link set mctppcc0 up
     # driver discovers max is (say) 500, now we have an invalid mtu

So, if you're able to parse the max from ACPI on initial bind, then you
can detect the error at the time of occurrence (the `link set mtu`)
rather than later (the `link set up`, and then ndo_open failing).

Check  me here: The channel has a value ring msg_count that keeps track 
of the number of entires in the ring buffer.  This needs to be set to0 
in order for it to think the buffer is empty.  It is initialized in  
__mbox_bind_client, called from mbox_bind_client which is in turn called 
from mbox_request_channel

The networking infra calls stop_ndo, so it must stop sending packets to 
it first.  I can netif_stop_queue(ndev) of course, but that seems 
counterintuitive? Assume i don't need to do that, but can't find the 
calling code.

You won't have any further ->ndo_start_xmit calls at the point that
->ndo_stop is called.

quoted

Is there a mbox facility to (synchronously) stop processing the inbound
channel, and completing the outbound channel?

There is mbox_free_channel which calls shutdown, and that removed the
IRQ handler, so no more  messages will be processed.  That should be 
sufficient.

OK, as above, this will depend on the approach you on allocating and
releasing the channels.

quoted

quoted

+       ndev->mtu = MCTP_MIN_MTU;
+       ndev->max_mtu = mctp_pcc_mtu;
+       ndev->min_mtu = MCTP_MIN_MTU;
+
+       rc = mctp_register_netdev(ndev, NULL, MCTP_PHYS_BINDING_PCC);
+       if (rc)
+               goto free_netdev;
+
+       return devm_add_action_or_reset(dev, mctp_cleanup_netdev, ndev);

As has been mentioned elsewhere, using the devm cleanup mechanism is a
bit unconventional here. You have the device remove callback available,
which lets you do the same, and that way you can demonstrate symmetry
between the add and remove implementations.

This has gone through a few  iterations and I thought I had it clear.

I was trying  to make use of automated cleanup as much as possible.

OK, your call there. Using ->remove allows you to be explicit about the
matching ordering, which would be my approach, but that's certainly not
the only correct one.

Cheers,


Jeremy