Hi Matan, On Tue, Sep 05, 2017 at 10:38:21AM +0000, Matan Azrad wrote: > Hi Adrien > > > -----Original Message----- > > From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com] > > Sent: Tuesday, September 5, 2017 12:28 PM > > To: Matan Azrad <ma...@mellanox.com> > > Cc: Nélio Laranjeiro <nelio.laranje...@6wind.com>; dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH v3] net/mlx5: support device removal event > > > > Hi Matan, > > > > On Mon, Sep 04, 2017 at 05:52:55PM +0000, Matan Azrad wrote: > > > Hi Adrien, > > > > > > > -----Original Message----- > > > > From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com] > > > > Sent: Monday, September 4, 2017 6:33 PM > > > > To: Matan Azrad <ma...@mellanox.com> > > > > Cc: Nélio Laranjeiro <nelio.laranje...@6wind.com>; dev@dpdk.org > > > > Subject: Re: [dpdk-dev] [PATCH v3] net/mlx5: support device removal > > > > event > > > > > > > > Hi Matan, > > > > > > > > One comment I have is, while this patch adds support for RMV, it > > > > also silently addresses a bug (see large comment you added to > > > > priv_link_status_update()). > > > > > > > > This should be split in two commits, with the fix part coming first > > > > and CC sta...@dpdk.org, and a second commit adding RMV support > > proper. > > > > > > > > > > Actually, the mlx4 bug was not appeared in the mlx5 previous code, > > > Probably because the RMV interrupt was not implemented in mlx5 before > > this patch. > > > > Good point, no RMV could occur before it is implemented, however a > > dedicated commit for the fix itself (i.e. alarm callback not supposed to > > end up > > calling ibv_get_async_event()) might better explain the logic behind these > > changes. What I mean is, if there was no problem, you wouldn't need to > > make > > priv_link_status_update() a separate function, right? > > > > The separation was done mainly because of the new interrupt implementation, > else, there was bug here. > The unnecessary alarm ibv_get_async_event calling was harmless in > the previous code. > I gets your point for the logic explanation behind these changes and I can > add it in this > patch commit log to be clearer, something like: > The link update operation was separated from the interrupt callback > to avoid RMV interrupt disregard and unnecessary event acknowledgment > caused by the inconsistent link status alarm callback.
Yes, it's better to explain why you did this in the commit log, but see below. > > > The big comment just explains the link inconsistent issue and was > > > added here since Nelio and I think the new function, > > > priv_link_status_update(), justifies this comment for future review. > > > > I understand, this could also have been part of the commit log of the > > dedicated commit. > > > Are you sure we need to describe the code comment reason in the commit log? It's a change you did to address a possible bug otherwise so we have to, however remember that a commit should, as much as possible, do exactly one thing. If you need to explain that you did this in order to do that, "this" and "that" can often be identified as two separate commits. Doing so makes it much easier for reviewers to understand the reasoning behind changes and leads to quicker reviews (makes instant-acks even possible). It'd still like a separate commit if you don't mind. -- Adrien Mazarguil 6WIND
