On Mon, 3 Jun 2019 11:04:36 +0200, Björn Töpel wrote:

On Sat, 1 Jun 2019 at 21:57, Jakub Kicinski
[off-list ref] wrote:

quoted

On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote:  

quoted

quoted

+   if (!bpf_op || flags & XDP_FLAGS_SKB_MODE)
+           mode = XDP_FLAGS_SKB_MODE;
+
+   curr_mode = dev_xdp_current_mode(dev);
+
+   if (!offload && curr_mode && (mode ^ curr_mode) &
+       (XDP_FLAGS_DRV_MODE | XDP_FLAGS_SKB_MODE)) {  

if i am reading this correctly this is equivalent to :

if (!offload && (curre_mode != mode))
offlad is false then curr_mode and mode must be DRV or GENERIC ..  

Naw, if curr_mode is not set, i.e. nothing installed now, we don't care
about the diff.
 

quoted

better if you keep bitwise operations for actual bitmasks, mode and
curr_mode are not bitmask, they can hold one value each .. according to
your logic..  

Well, they hold one bit each, whether one bit is a bitmap perhaps is
disputable? :)

I think the logic is fine.
 

Hmm, but changing to:

       if (!offload && curr_mode && mode != curr_mode)

is equal, and to Saeed's point, clearer. I'll go that route in a v3.

Sorry, you're right, the flags get mangled before they get here, so
yeah, this condition should work.  Confusingly.

quoted

What happened to my request to move the change in behaviour for
disabling to a separate patch, tho, Bjorn? :)  

Actually, I left that out completely. This patch doesn't change the
behavior. After I realized how the flags *should* be used, I don't
think my v1 change makes sense anymore. My v1 patch was to give an
error if you tried to disable, say generic if drv was enabled via
"auto detect/no flags". But this is catched by looking at the flags.

What I did, however, was moving the flags check into change_fd so that
the driver doesn't have to do the check. E.g. the Intel drivers didn't
do correct checking of flags.

Ugh.  Could you please rewrite the conditions to make the fd >= check
consistently the outside if?  Also could you add extack to this:

+	if (!offload && dev_xdp_query(dev, mode) &&
+	    !xdp_prog_flags_ok(dev->xdp_flags, flags, extack))
+		return -EBUSY;

It's unclear what it's doing.