Add a script to install bluetooth

[tobiasBora]

This script do all the job to patch the btusb file in a clever way, compiling the kernel, and copying the btusb.ko file in /lib/.... It also save the old btusb.ko and provide an easy way to recover it with ./uninstall.

I just wonder if doing this can break secure boot systems. If yes I can add a message at the beginning saying "please be sure to disable secure boot before installing bluetooth".

[neurobin]

Several problems:

install script:

echo "Wifi has been successfully installed."

Even if the make and subsequent command fails, it will still show "successfully installed", this echo needs to be ANDed to the previous commands:

make &&
make install &&
echo "install success"
modprobe mt7630e &&
echo "wifi module loaded"
modprobe mt76xx &&
echo "bluetooth module loaded"

install_bluetooth:

interestingLine=$(cat btusb.c | grep -e "#define BTUSB_.0x[1248]0$" | tail -1)

1. The patten "#define BTUSB_.*0x[1248]0*$" is not intelligent enough. We don't need a line that is commented out (not expected though) e.g ( /*#define BTUSB...*/), so pattern must ensure that it at least starts from the beginning of the line ( ^[[:blank:]]*#define ...).
2. After the hex id .*0x[1248]0* there can be unintentional white spaces, i.e it needs to be .*0x[1248]0*[[:blank:]]*$
3. If BTUSB_MEDIATEK is already added then this step should be skipped totally either by the pattern or a conditional.

sed -i "s/$interestingLine/$interestingLine\n#define BTUSB_MEDIATEK $newNumber/g" btusb.c

1. This adds the #define every time regardless if it already exists or not. There should be a test to see if it's already added.
2. s/$interestingLine/$interestingLine\n... can be compacted to s/$interestingLine/&\n
3. the g modifier is redundant, there should be only one #define BTUSB_MEDIATEK in the entire file.

sed -i "s/{ USB_DEVICE(0x0e8d, 0x763f) },/{ USB_DEVICE(0x0e8d, 0x763f), .driver_info = BTUSB_MEDIATEK },/g" btusb.c

1. redundant g modifier

sed -i "s/set_bit(HCI_QUIRK_BROKEN_LOCAL_COMMANDS, &hdev->quirks);/set_bit(HCI_QUIRK_BROKEN_LOCAL_COMMANDS, &hdev->quirks);\n }\n if (id->driver_info & BTUSB_MEDIATEK) {\n set_bit(HCI_QUIRK_BROKEN_LOCAL_COMMANDS, &hdev->quirks);\n/g" btusb.c

1. & in replace part need to be escaped
2. sed isn't the right tool for this. Targetting a single line isn't that secure. this will break, even if the sequence of the line becomes diffenent . awk would be a better choice, though sed can do this too, but sed expression will be too much complex to follow through.

3 . can be compacted with &

[neurobin]

I think targeting the if (id->driver_info & BTUSB_INTEL) { line will be better (thus a simple sed would be enough):

patch='\n\tif (id->driver_info \& BTUSB_MEDIATEK) { \n\t\tset_bit(HCI_QUIRK_BROKEN_LOCAL_COMMANDS, \&hdev->quirks);\n\t}\n'
if ! grep -A2 -e 'if (id->driver_info & BTUSB_MEDIATEK)' btusb.c |
     grep -e 'set_bit(HCI_QUIRK_BROKEN_LOCAL_COMMANDS, &hdev->quirks);' >/dev/null; then
    sed -i "s/^[[:blank:]]*if[[:blank:]]*(id->driver_info & BTUSB_INTEL)/$patch\n&/" btusb.c
fi

[tobiasBora]

I did all the modifications expected, but the last one: I don't understand why

if (id->driver_info & BTUSB_INTEL)

is interesting: the patch talks about a line that ends with BOOT!

if (id->driver_info & BTUSB_INTEL_BOOT)

And if we focus on this line we can see that there are two lines that match

if (id->driver_info & BTUSB_INTEL_BOOT)

[tobiasBora]

(and you can note that the "set -e" option avoid the use of &&)

[neurobin]

if (id->driver_info & BTUSB_INTEL) is in the same block of code, and it has no duplicate. And these are all if blocks independent of each other, that's why adding it before or after another if block is the same as long as it remains in an appropriate place.

[neurobin]

For example,

if a = 1 then do something1
if a = 2 then do something2
if a = 3 then do something3
if a = 4 then do something4

is the same as:

if a = 4 then do something4
if a = 1 then do something1
if a = 2 then do something2
if a = 3 then do something3

because a is unique and each if block is independent of each other.

[neurobin]

interestingLine=$(cat btusb.c | grep -e "^[[:blank:]]#define BTUSB_.0x[1248]0[[:blank:]]$" | tail -1)

useless use of cat, grep can take file as arg: grep -e pat file

interestingNumber=$(echo $interestingLine | sed 's/^.(0x[0-9])$/\1/g')

$interestingLine may contain unintentional white space at end, either it needs to be stripped or the sed pattern can be changed to ^.*\(0x[0-9]*\)[[:blank:]]*$ so that interestingNumber doesn't contain any white space.

$interestingLine in subshel should be quoted: $(echo "$interestingLine"...

sed -i "s/set_bit(HCI_QUIRK_BROKEN_LOCAL_COMMANDS, \&hdev->quirks);/

escaping & in search part isn't required (only in replace part), no harm though...

[neurobin]

And targeting set_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks); is kinda dangerous. For example, if there was a line after it:

if (id->driver_info & BTUSB_INTEL_BOOT) {
        hdev->manufacturer = 2;
        set_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks);
        another line for BTUSB_INTEL_BOOT block;
    }

then it would become like this (with your code):

if (id->driver_info & BTUSB_INTEL_BOOT) {
        hdev->manufacturer = 2;
        set_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks);
        }
if (id->driver_info & BTUSB_MEDIATEK) {
        set_bit(HCI_QUIRK_BROKEN_LOCAL_COMMANDS, \&hdev->quirks);
        another line for BTUSB_INTEL_BOOT block;
    }

[tobiasBora]

Yes, I agree that mine solution isn't perfect, it works especially because the file is like he is now.

Well it's possible to exchange two if only if "id->driver_info" contains a single bit set to one... And sometimes it seems to be possible to have several non null bites:

.driver_info = BTUSB_INTEL_BOOT | BTUSB_BROKEN_ISOC

After, BTUSB_MEDIATEK is never "anded" with something else, so I think it should work if the will stay like this.

I have implemented your way to proceed, see the above patch.

[neurobin]

$interestingLine in subshel should be quoted: $(echo "$interestingLine"...

Sorry if I mislead you with this. I meant in line:

interestingNumber=$(echo $interestingLine | sed 's/^.*\(0x[0-9]*\)[[:blank:]]*$/\1/g')

should be:

interestingNumber=$(echo "$interestingLine" | sed 's/^.*\(0x[0-9]*\)[[:blank:]]*$/\1/g')

newNumber=$(printf "0x%X\n" $(($(echo "$interestingNumber") + $(echo "$interestingNumber")))) is too much redundant. It was fine as it was:

newNumber=$(printf "0x%X\n" $(($interestingNumber + $interestingNumber)))

[tobiasBora]

I'm not sure to see why quoting is needed here, but it's not expensive to write... Anyway, is it ok like this ?

[neurobin]

As $interestingLine doesn't contain anything that can go through globbing, it's ok without the quote, but it's a good practice to always be cautious of possible danger codes.

[neurobin]

well done :+1:

I will give it a final review and push it to release branch when I get the time.. :smile:

[neurobin]

I want to shorten the name a little, how about bpatch (bluetooth patch) for the install script and ubpatch for the uninstall script. And also, I wish to mention you as the author of the script (inside the script) if that's not a problem.

[neurobin]

I have pushed into master branch modifying your script.

1. named it bpatch for now
2. it can both apply or undo the patch i.e the uninstall_bluetooth script is integrated with the bpatch script using a function
3. updated the doc

You can check and review it, and do make suggestions if you have any. :+1:

[tobiasBora]

Well the review is very clean, so everything is ok for me. I just find the name "bpatch" a little bit less clear, but since the documentation is explicit it doesn't cause any problem for me.

Thank you !

Le 2 juillet 2016 14:08:21 GMT+02:00, Md Jahidul Hamid notifications@github.com a écrit :

I have pushed into master branch modifying your script.

1. named it bpatch for now
2. it can both apply or undo the patch i.e the uninstall_bluetooth script is integrated with the bpatch script using a function
3. updated the doc

---

You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/neurobin/MT7630E/pull/32#issuecomment-230098720

TobiasBora